Skip to content

[EuiSuggest] Fire onItemClick callback on Enter key presses; [EuiSelectable] Fix Enter and Space key to select items on non-searchable listboxes#5693

Merged
cee-chen merged 7 commits intoelastic:mainfrom
cee-chen:suggest-selectable-enhancements
Mar 8, 2022

Conversation

@cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Mar 4, 2022

Summary

This is a PR in two slightly separate parts. I recommend following along by commit; the first 2 commits deal with EuiSuggest, and the last 2 commits deal with EuiSelectable (which somewhat floats up to affect EuiSuggest). If requested I can break these up into 2 separate PRs for EuiSuggest and EuiSelectable.

Release considerations

I strongly feel this PR should get into a patch/backport release and be added to elastic/kibana#126926 for two reasons:

  1. @spong has requested this EuiSuggest behavior by 8.2 FF, and
  2. the broken Enter/Space to select behavior for non-searchable EuiSelectables is a huge regression for keyboard users.

QA

euisuggest

EuiSuggest

  • Go to https://eui.elastic.co/pr_5693/#/forms/suggest
  • Open devtools to console output
  • Click the first suggest input
  • Using only your keyboard, use the up/down arrow keys and then the Enter key to select an item
  • In your console devtools, confirm that the selected item's suggestion object was returned
  • (Bonus regression testing) Click any other item and confirm that that clicked item's suggestion obj was logged
  • (Bonus 4th commit testing) After clicking an item, continue to use the up/down arrow keys and Enter to select items, and confirm that that still works as expected

EuiSelectable

  • Go to https://eui.elastic.co/pr_5693/#/forms/selectable
  • Use the tab key to enter the first EuiSelectable and then up/down arrow keys to navigate to items
  • Confirm that the Enter key works to check/uncheck items (unlike current production)
  • Confirm that the Space key works to check/uncheck items
  • (Bonus regression testing) Go to the searchable demo right below the first one and confirm that the arrow/Enter key still works as expected
  • (4th commit testing) Using your mouse, click any item in the searchable list. Then switch to using your arrow keys to navigate through items and confirm that pressing Enter/Space now works to toggle items.

Checklist

- [ ] Check against all themes for compatibility in both light and dark modes
- [ ] Checked in mobile
- [ ] Checked in Chrome, Safari, Edge, and Firefox
- [ ] Props have proper autodocs and playground toggles
- [ ] Added documentation
- [ ] Checked Code Sandbox works for any docs examples

- [ ] Checked for breaking changes and labeled appropriately

  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

cee-chen added 4 commits March 4, 2022 13:01
- under the hood, this uses EuiSelectable's onChange callback to capture item selection events
…iSelectable

- since onChange handles this for us automatically, we no longer need to manually pass an onClick handler

+ remove logic for conditional `button` styling - as far as I can tell this was no longer being used/in play since the EuiSelectable refactor
…n non-`searchable` listboxes

+ minor comment reorganization

+ add Cypress tests to ensure we don't get regressions on this in the future
… users when selecting items

- This enhancement allows users who click a list item and then still want to use keyboard nav or enter/space keys to select a list item to do so

- Prevoiusly, the event.target checks were preventing this from occurring
@cee-chen cee-chen requested a review from thompsongl March 4, 2022 21:22
@cee-chen cee-chen marked this pull request as ready for review March 4, 2022 21:27
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5693/

@spong
Copy link
Member

spong commented Mar 5, 2022

Thank you so much for speedy efforts here @constancecchen!

Since there were so many changes to EuiSuggest since 48.1.1 I tested a bit with the 50.0.0 version in a codesandbox, and I'm starting to wonder if I may be expecting a bit more out of this component than what may be intended... 😬

In addition to the the suggestion selection changes you've implemented here, I'm seeing a need for quite a bit more control around some of the internal components. For instance:

  1. When a suggestion item is selected, we'll want to append its label to the existing input value (now fixed in 50.0.0 🙌), set the cursor at the end of the input, and dismiss the suggestion popover (or perhaps like the Discover KQL bar, show operator suggestions). Maybe this should happen automatically after the onItemClick(?), but if not there'd need to be a callback for dismissing the suggestion popover and re-focusing.

  2. Multi-sugggestion support. Once the above happens, say a user selects the field suggestion indexDuration, we'd want to either show applicable operator suggestions (:, >, <) or keeping it simpler, only start suggesting again after a space is detected (i.e. indexDuration > 0 start showing suggestions again). Right now the suggestion popover stays active saying indexDuration > 0 doesn't match any options.

So at this point, the LoE/scope here starts ballooning 🎈 quite a bit, and this component would need to start being waaaay more opinionated, and would just be marching towards Kibana's data.ui plugin's searchbar component, which appears to be a completely custom implementation (I'm not seeing any usages of EuiSuggestion(Item) in Kibana right now 😔).

All that said, I want to be conscious of your-all's time, and it seems like I should either go the custom component route (mirroring the suggestion UI like the data.ui plugin does), or propose the necessary changes to the Kibana searchbar in support of working around the RBAC restrictions of the datasource I'm working with. However, if there is any interest in supporting some of the above mentioned functionality in this component, I'm happy to provide further feedback, just let me know! 🙂 Thanks again!

Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

One small nit in a test file, otherwise this LGTM; tested in the preview build, including the steps included in the PR description; also pulled & poked at the code locally.

For the release consideration, we shouldn't have any problem getting the next EUI release into Kibana before FF; unless there's some needed coordination with changes in Kibana holding off on a backport should be fine. We can also do the backport if it helps any coordination or avoids creating stress somewhere.

@cee-chen cee-chen force-pushed the suggest-selectable-enhancements branch from cf3ca75 to c5cd06b Compare March 7, 2022 22:44
- While keeping EuISuggest's onClick cleanup
- note that resulting types shenanigans makes passing just EuiSuggestItem no longer possible, so I opted to just pass in an inline fn
@cee-chen cee-chen force-pushed the suggest-selectable-enhancements branch from c5cd06b to f9e9b54 Compare March 7, 2022 22:45
@cee-chen cee-chen requested a review from thompsongl March 7, 2022 22:46
@kibanamachine
Copy link

Preview documentation changes for this PR: https://eui.elastic.co/pr_5693/

Copy link
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

Functionality confirmed with the QA steps (thanks for those!)

I strongly feel this PR should get into a patch/backport release and be added to elastic/kibana#126926

We may need a patch for ^ release anyway. We can coordinate after I have a little more info.

@cee-chen
Copy link
Contributor Author

cee-chen commented Mar 8, 2022

Sounds great, thanks Greg!! 👍

@cee-chen cee-chen merged commit da1a5fa into elastic:main Mar 8, 2022
@cee-chen cee-chen deleted the suggest-selectable-enhancements branch March 8, 2022 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants