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

[EuiSelectable] Fix searchable single selection selectables not correctly highlighting the checked option on initial render #6072

Merged
merged 4 commits into from
Jul 25, 2022

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Jul 22, 2022

Summary

This PR also contains 2 other small fixes / cleanups (see commit history).

Before

before

After

after

Problem

EuiSelectable.onSearchChange was being called on mount by EuiSelectableSearch, which was setting activeOptionIndex to undefined:

onSearchChange = (
searchValue: string,
visibleOptions: Array<EuiSelectableOption<T>>
) => {
this.setState(
{
searchValue,
visibleOptions,
activeOptionIndex: undefined,
},

Solution

Moving the searchProps.onChange callback to the EuiSelectable's constructor (which calls getMatchingOptions/sets search state on mount anyway, so no need for EuiSelectableSearch to repeat it) fixes the issue and leaves the found activeOptionIndex from the constructor intact.

Checklist

- [ ] Checked 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
- [ ] Updated the Figma library counterpart
- [ ] Checked for accessibility including keyboard-only and screenreader modes Tested but no meaningful impact for these users - they have to tab into the Selectable which triggers the highlight in any case

cee-chen added 4 commits July 22, 2022 09:33
…leSelection` `searchable` EuiSelectables

`Euiselectable.onSearchChange` being called on mount by `EuiSelectableSearch` which was setting `activeOptionIndex` to undefined. Moving the call to the `EuiSelectable.constructor` (which calls getMatchingOptions on mount anyway, so no need for `EuiSelectableSearch` to repeat it fixes the issue and leaves the found `activeOptionIndex` from the constructor intact.
…ionIndex` is a 0 index

since 0 is falsey but we really should be checking for undefined
- doesn't appear to be used anywhere
@kibanamachine
Copy link

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

@cee-chen cee-chen requested a review from thompsongl July 25, 2022 14:51
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.

LGTM!

@cee-chen cee-chen merged commit 4018c82 into elastic:main Jul 25, 2022
@cee-chen cee-chen deleted the selectable-bugfixes branch July 25, 2022 15:52
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.

3 participants