-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
PeoplePicker: Improved keyboard support #2372
PeoplePicker: Improved keyboard support #2372
Conversation
…size, made suggested contact list properly focus first suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All looks good to me, but I'll defer to someone who's worked inside peoplepicker more
The second issue is fixed, testing with Narrator + Edge, but I can't get the "Load all results" footer to appear. |
Ok, I talked to to Kris, and Top 10 results is just a label. Please make that label color instead of link color "neutralPrimary" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to change "Top 10 results" to a non-link neutralPrimary color
…/kristoferbrown/office-ui-fabric-react into v-krbrow/peoplepicker-owa-review
@betrue-final-final, I removed the Top 10 element since it wasn't really represented in the XD file and was causing this ambiguity. |
@@ -25,6 +25,7 @@ $Callout-smallbeak-slant-width: 16px; | |||
@include ms-drop-shadow(0,0, 5px, 0px); | |||
position: absolute; | |||
box-sizing: border-box; | |||
border: 1px solid $ms-color-neutralLight; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As much as I want this change to happen, this isn't the right PR for it and I'd recommend splitting it out into it's own PR in an effort to keep PRs Single Responsibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The border was originally removed in an update to the people picker's callout, so I didn't think it was a big deal to add it back in another update to the people picker's callout. You're right though, I'll pull it out here and start a new PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request checklist
$ npm run change
Description of changes
Allows the 'Load all Results' button to get keyboard focus.
Prevents keyboard focus from skipping the first item in the suggested contacts list.
Also, decreases font size of Loading and Load all Results elements.
Focus areas to test
The people picker suggestion list.