-
Notifications
You must be signed in to change notification settings - Fork 819
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
[EuiSuggest] Use EuiSelectable
#5157
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
I had an idea for a custom hook that would provide stateful logic and ARIA attributes needed to make combox-like components that we could use here and elsewhere. Going to look into that before determining the best path forward for this PR. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
@1Copenut Before I open this up for full review, would you mind checking this for a11y completeness? I'd like to have that part solid before making further review adjustments. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
Looks like a focus trap change in #5584 had an impact here. Still trying to diagnose. Update: |
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
Maybe these could be removed in non-virtualized instances? Perhaps @cchaos has thoughts.
This is a noted limitation of virtualized
Yep, separate PR makes sense. In that same PR I can resolve @1Copenut concern about the input clear button not responding to ENTER. Both of these are
You could |
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.
👍 I retested for keyboard and screen reader behavior with VO + Safari on MacOS Big Sur. The focus handling is spot on and the reduced set of options when I started typing worked well too. VO tends to be fairly verbose and repetitive, so I have no doubt this will be a big upgrade for NVDA and JAWS too. Thanks @thompsongl !
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.
Mostly minor/optional comments, and a few maybe-blocking comments about unit tests
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.
Did a real quick final code and design review. LGTM!
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
|
||
describe('EuiSuggest', () => { | ||
describe('onItemClick', () => { | ||
it('is called when an option is clicked', () => { |
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.
🤩 Nice!! These Cypress tests are great!!! Thanks for adding them!
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.
New tests look awesome! I have just one super minor/opinionated code style suggestion, feel free to ignore it and merge if you don't think it's a significant improvement
Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_5157/ |
* update euiselectable with paddingSize prop, tests * euisuggest use euiselectable; more tests * CL * handle empty list * WIP: useCombobox * WIP: temporary docs * snapshots * keyboard nav * i18n * prevent prop overlap * open logic * status i18n * clean up * aria-checked; empty state * restrict aria-checked to role=option items * checked screenreader text * WIP: EuiScreenReaderStatus * i18n token * i18n fix * filter out disabled items * space bar toggles selection in non-searchable euiselectable * update tri-state helper text * fix dupe token * euiScreenReaderStatus * optional virtualized rendering, use in EuiSelectable * fix CL * expose isVirtualized option * CL * Fixed tall lists by added `maxHeight` prop * Fix a few item styles * Fix height for `isVirtualized` and add `title`s to spans * [EuiSelectable] Don’t render unnecessary wrappers * More cleanup of item code and styles Deprecates `descriptionDisplay` and `labelDisplay` * Fixing `fullWidth` support * Docs * import * 1.2 spec * pass aria attrs to input * clean up * fix CL * add key to non-virtualized list items * remove unused component; rename sendValue prop * Refactor to simplify EuiScreenReaderStatus, support more panel prop configurations in EuiPopover, and rely on new panel props support in EuiSuggest * clean up * Fix input style when append array is empty; Update docs for suggest item * euiscreenreaderonly * euiscreenreaderstatus * tests, comments * Apply suggestions from code review Co-authored-by: Constance <constancecchen@users.noreply.github.com> * merge fixes * clean up * docs clean up * docs clean up * id pattern fix * fix bad merge * disabled redundant focus trap * docs and tests * move state message * Update src/components/suggest/suggest.tsx Co-authored-by: Constance <constancecchen@users.noreply.github.com> * lint Co-authored-by: cchaos <caroline.horn@elastic.co> Co-authored-by: Chandler Prall <chandler.prall@gmail.com> Co-authored-by: Constance <constancecchen@users.noreply.github.com>
Summary
Resolves #3733 by converting the list element in
EuiSuggest
to useEuiSelectable
, which now implements the ARIA 1.2 combobox pattern.Other changes:
EuiSelectable
does the heavy liftingEuiSuggestInput
aria-label
oraria-labelledby
sendValue
prop toonSearchChange
Closes #5336, closes #2404, closes #4419, closes #4345, closes #4342
Checklist
- [ ] Checked in mobile