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

[EuiSearchBar] Fix multiSelect: false filter behavior #6577

Merged
merged 3 commits into from
Feb 6, 2023

Conversation

cee-chen
Copy link
Contributor

@cee-chen cee-chen commented Feb 3, 2023

Summary

closes #6540
closes #6569

Huge shout out to @robert-seymour-numerated for extremely accurately narrowing down the location in our code in which this bug was occurring, saving me a whole bunch of time 🙌

And another bonus shout out to @Heenawter for requesting the new changedOption parameter/API that we added in #6487, which made this fix extremely easy and require 0 array iteration/comparison 🎉

I recommend checking the git commit messages for more context & detail as to how the fix works.

Before

before

After

after

QA

The buggy behavior can be reproduced/compared against production docs.

Regression testing

General checklist

  • Checked in Chrome, Safari, Edge, and Firefox
  • Added or updated jest and cypress tests
  • A changelog entry exists and is marked appropriately

- the diff being returned was catching on the first item (now unflagged) and sending that instead

- instead of using an unnecessary `.find`, use our new 3rd `changedOption` arg from `EuiSelectable` to immediately get the changed/selected option

- nb: this does require flipping all ternaries on `onOptionClick` b/c the new `changedOption` correctly sends the new updated state
@cee-chen cee-chen marked this pull request as ready for review February 3, 2023 00:30
@cee-chen
Copy link
Contributor Author

cee-chen commented Feb 3, 2023

Pinging either @breehall or @JasonStoltz for a review - I'm hoping to get this fix in by 8.7FF next Tuesday, but that maay be a pipe dream

@kibanamachine
Copy link

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

Copy link
Contributor

@breehall breehall left a comment

Choose a reason for hiding this comment

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

Approved! I pulled and tested locally with the in-memory table example and compared it to production. Also ran the new regression tests in Cypress locally and everything passing. Awesome fix Cee!

@cee-chen cee-chen merged commit 3c2a9a9 into elastic:main Feb 6, 2023
@cee-chen cee-chen deleted the multiselect-false-fix branch February 6, 2023 19:40
cee-chen added a commit to elastic/kibana that referenced this pull request Feb 9, 2023
## Summary

`[email protected]` ⏩ `[email protected]`

___

## [`74.0.2`](https://github.com/elastic/eui/tree/v74.0.2)

**Bug fixes**

- Fixed `EuiCard` to ensure `onClick` method only runs once when `title`
contains a React node
([#6551](elastic/eui#6551))
- Fixed `EuiSelectable` options with incorrect `aria-posinset` indices
when rendered with group labels not at the start of the array
([#6571](elastic/eui#6571))
- Fixed a bug with `EuiSearchBar` where filters with `multiSelect:
false` were not able to select a new option when an option was already
selected ([#6577](elastic/eui#6577))

Co-authored-by: Kibana Machine <[email protected]>
1Copenut added a commit to elastic/kibana that referenced this pull request Feb 14, 2023
## Summary

`[email protected]` ⏩ `[email protected]`

___

## [`75.0.0`](https://github.com/elastic/eui/tree/v75.0.0)

- `EuiFlyout`s now automatically shard all fixed `EuiHeader`s on the
page. This means that interactions (mouse & keyboard) with items inside
`EuiHeader`s when flyouts are open will no longer trigger focus fighting
([#6566](elastic/eui#6566))
- `EuiFlyout`s now read out detailed screen reader dialog instructions
and hints on open ([#6566](elastic/eui#6566))

**Bug fixes**

- Fixed `EuiSelectable` options with incorrect `aria-posinset` indices
when rendered with group labels not at the start of the array
([#6571](elastic/eui#6571))
- Fixed a bug with `EuiSearchBar` where filters with `multiSelect:
false` were not able to select a new option when an option was already
selected ([#6577](elastic/eui#6577))

**Breaking changes**

- Removed the ability to customize the `role` prop of `EuiFlyout`s.
`EuiFlyout`s should always be dialog roles for screen reader
consistency. ([#6566](elastic/eui#6566))
- Removed `closeButtonAriaLabel` prop from `EuiFlyout` - use
`closeButtonProps['aria-label']` instead
([#6566](elastic/eui#6566))

---------

Co-authored-by: Kibana Machine <[email protected]>
justinkambic pushed a commit to justinkambic/kibana that referenced this pull request Feb 23, 2023
## Summary

`[email protected]` ⏩ `[email protected]`

___

## [`75.0.0`](https://github.com/elastic/eui/tree/v75.0.0)

- `EuiFlyout`s now automatically shard all fixed `EuiHeader`s on the
page. This means that interactions (mouse & keyboard) with items inside
`EuiHeader`s when flyouts are open will no longer trigger focus fighting
([elastic#6566](elastic/eui#6566))
- `EuiFlyout`s now read out detailed screen reader dialog instructions
and hints on open ([elastic#6566](elastic/eui#6566))

**Bug fixes**

- Fixed `EuiSelectable` options with incorrect `aria-posinset` indices
when rendered with group labels not at the start of the array
([elastic#6571](elastic/eui#6571))
- Fixed a bug with `EuiSearchBar` where filters with `multiSelect:
false` were not able to select a new option when an option was already
selected ([elastic#6577](elastic/eui#6577))

**Breaking changes**

- Removed the ability to customize the `role` prop of `EuiFlyout`s.
`EuiFlyout`s should always be dialog roles for screen reader
consistency. ([elastic#6566](elastic/eui#6566))
- Removed `closeButtonAriaLabel` prop from `EuiFlyout` - use
`closeButtonProps['aria-label']` instead
([elastic#6566](elastic/eui#6566))

---------

Co-authored-by: Kibana Machine <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants