Skip to content
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

Featurebug/combobox input focus #1258

Merged

Conversation

chandlerprall
Copy link
Contributor

@chandlerprall chandlerprall commented Oct 23, 2018

Summary

fixes #1195 fixes #1251 closes #589

  • escape key now closes an open combo box
  • keeps browser focus on the combo box's input instead of shifting focus to the popover options
  • aria roles for screen reading
  • compressed prop added to EuiComboBox ( @cchaos please verify & review this )
  • hides the dropdown icon if noSuggestions is present

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • [ ] This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios

@cchaos
Copy link
Contributor

cchaos commented Oct 24, 2018

@chandlerprall , I'm not sure if you saw but I pushed a PR against your branch with some more fixes: chandlerprall#2

@snide
Copy link
Contributor

snide commented Oct 24, 2018

The ownFocus popovers can sometimes cause problems with the esc from a combo box. You end up in a loop. I don't know a good solution for this one.

- Fixed single selection truncation
- Actually apply `compressed` styles
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

This looks great overall! I had a few comments but I also think the UX of using ESC to open and close the options list to be slightly improved. I think the existing UX is confusing because ESC causes your focus to vanish from the user's perspective. What do you think of mimicking the UX of react-select whereby ESC closes the options list but retains focus on the input? The user can reopen the input by hitting an arrow key or enter. This way as a user I'd feel more in control of where the focus is.

CHANGELOG.md Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
src/components/combo_box/combo_box.js Show resolved Hide resolved
@chandlerprall
Copy link
Contributor Author

chandlerprall commented Oct 25, 2018

@snide what do you think about CJ's Escape key comment (copied relevant portion here)?

I think the existing UX is confusing because ESC causes your focus to vanish from the user's perspective. What do you think of mimicking the UX of react-select whereby ESC closes the options list but retains focus on the input?

That would resolve the popover esc->popup->esc->popup loop. I went back and forth on if Escape should remove focus or not, and am not opinionated either way.

@snide
Copy link
Contributor

snide commented Oct 25, 2018

@chandlerprall Sounds good to me. Good idea @cjcenizal

@chandlerprall
Copy link
Contributor Author

@cjcenizal everything should be addressed

Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Tested in browser, UX of Escape key is great! Thanks for the changelog updates, too. Nice work!

@chandlerprall chandlerprall merged commit 9e429db into elastic:master Oct 30, 2018
@chandlerprall chandlerprall deleted the featurebug/combobox-input-focus branch October 30, 2018 16:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants