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 6 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={ FocusZoneDirection.horizontal } >

@joschect joschect Apr 12, 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.

David mentioned that we really shouldn't be setting this to be just horizontal. If we need to fix alignment or something like that then we should do that.

@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.

If that is the case I can back this portion out, however there are a couple issues that will need attention if we don't switch this to horizontal:

  1. Even with Edge's alignment issues fixed, the underlying issue still makes some of the elements unfocusable.
  2. In all browsers, keyboarding is completely broken in the example custom document picker on the Pickers component page. Only the first selected document is focusable and none of the internal elements can be selected.

Switching to horizontal remedies both of these problems, but at the expense of the up and down keys. So it may be a cleaner, more permanent solution to address these two issues separately instead of using horizontal here. My suspicion is that issue 1 is an Edge bug and issue 2 may be a problem with that specific picker implementation.

{ 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.

}