Skip to content

People picker edge keyboarding #1496

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kristoferbrown merged 11 commits into from
Apr 26, 2017
Merged

People picker edge keyboarding #1496

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
1a84572
Adding vertical-align and horizontal only keyboarding to the member l…
Apr 11, 2017
7b7ce7b
Rush change.
Apr 11, 2017
48b8b56
Merge branch 'master' into people-picker-edge-keyboarding
kristoferbrown Apr 11, 2017
95e1bb0
Using FocusZoneDirection enum instead of just the number.
Apr 11, 2017
d50220f
Merge branch 'people-picker-edge-keyboarding' of https://github.com/k…
Apr 11, 2017
4089137
Merge branch 'master' into people-picker-edge-keyboarding
kristoferbrown Apr 11, 2017
62cb3d5
Merge branch 'master' into people-picker-edge-keyboarding
micahgodbolt Apr 18, 2017
b21e8d2
Merge branch 'master' into people-picker-edge-keyboarding
kristoferbrown Apr 25, 2017
76cc42a
Removing horizontal only param from the member list people picker foc…
Apr 25, 2017
d261ffe
Merge branch 'master' into people-picker-edge-keyboarding
kristoferbrown Apr 25, 2017
759b02d
Merge branch 'master' into people-picker-edge-keyboarding
kristoferbrown Apr 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions common/changes/people-picker-edge-keyboarding_2017-04-11-22-49.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"changes": [
{
"packageName": "office-ui-fabric-react",
"comment": "PeoplePicker: Adding vertical-align styles and horizontal only keyboarding to the member list people picker focus zone.",
"type": "patch"
}
],
"email": "v-krbrow@microsoft.com"
}
3 changes: 2 additions & 1 deletion packages/office-ui-fabric-react/src/components/pickers/BasePicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -438,7 +438,8 @@ export class BasePickerListBelow<T, P extends IBasePickerProps<T>> extends BaseP
{ this.renderSuggestions() }
<FocusZone ref={ this._resolveRef('focusZone') }
className='ms-BasePicker-selectedItems'
isInnerZoneKeystroke={ this._isFocusZoneInnerKeystroke } >
isInnerZoneKeystroke={ this._isFocusZoneInnerKeystroke }
direction={ 1 } >

@MLoughry MLoughry Apr 11, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use the FocusZoneDirection enum, rather than a magic number.

@kristoferbrown kristoferbrown Apr 11, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call, this has been changed.

@kristoferbrown kristoferbrown Apr 25, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@MLoughry, this portion of the change has been completely removed. Can you take another look and approve here?

{ this.renderItems() }
</FocusZone>

Expand Down
1 change: 1 addition & 0 deletions ...abric-react/src/components/pickers/PeoplePicker/PeoplePickerItems/PickerItemsDefault.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,4 +44,5 @@

.itemContainer {
display: inline-block;
vertical-align: top;

@micahgodbolt micahgodbolt Apr 11, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really necessary?

@kristoferbrown kristoferbrown Apr 12, 2017

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is in Edge. Without it, items can get out of alignment as you keyboard through them.

@dzearing dzearing Apr 21, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes it is!!! any time you inline-block, you should strongly, strongly consider setting vertical alignment. Otherwise the browser will shuffle the div around trying to align its containing text with other text.

}