-
Notifications
You must be signed in to change notification settings - Fork 860
[EuiInMemoryTable] Support controlled plain text search #9142
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
[EuiInMemoryTable] Support controlled plain text search #9142
Conversation
- don't throw when an invalid query like "or" is passed in search.query - ensure controlled behavior by ensuring search.query is passed down to the search bar
ebe0bf2 to
9967c96
Compare
rmyz
left a comment
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.
Tested against local kibana with elastic/kibana#233836 and it works fine, thanks!
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.
✅ The updated search.query and search.defaultQuery props work as expected:
queryno longer results in an error forsearchFormat="text"defaultQueryis properly applied
I have one doubt about a possible edge case, otherwise the changes LGTM.
namely handling also defaultQuery, and also improve JSDoc
💚 Build SucceededHistory
cc @acstll |
💚 Build Succeeded
History
cc @acstll |
mgadewoll
left a comment
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.
🟢 Thanks for the additional changes! The code LGTM and the updated search.query and search.defaultQuery work as expected 👍
this is reverting a change introduced in elastic#9142 for EuiSearchBarOnChangeArgs, where the query prop could be null also when there was no error, requiring additional checks in consumer code
## Dependency updates - `@elastic/eui`: `v109.2.0` ⏩ `v110.0.0` --- ## Changes - Updates usages of the `message` prop on `EuiInMemoryTable` to `noItemsMessage` - Updates usages of `EuiSearchBar`'s `onChange` prop to check for the `query` argument, as it may be `null` now - Removes obsolete `color` props on implementations of `EuiPageTemplate` and `EuiPageTemplate.Header` (`color` prop wasn't applied before either) ## Package updates ### `@elastic/eui` v110.0.0 - Updated `EuiSuperDatePicker` to expose plain text `utcOffset` and `timeZoneName` in `timeZoneDisplayProps.customRender` render function ([#9245](elastic/eui#9245)) - Updated `EuiHeaderLogo` to add a hover style ([#9240](elastic/eui#9240)) - Made `EuiQuickSelectPanel` available for importing from the `@elastic/eui` package ([#9239](elastic/eui#9239)) - Updated `EuiAvatar` to support Emoji Sequence ("advanced") in the `initials` prop ([#9227](elastic/eui#9227)) - Added `color` prop on `EuiPageHeader` to support `transparent` (default) and `plain` backgrounds. ([#9220](elastic/eui#9220)) - Added `color` prop on `EuiPage` to support `transparent` (default) and `plain` backgrounds. ([#9220](elastic/eui#9220)) - Updated `EuiPageTemplate` to ensure `panelled=true` renders a `EuiPageHeader` with a plain background by default ([#9220](elastic/eui#9220)) - Removed the default background style on `EuiPageTemplate`'s outer wrapper ([#9220](elastic/eui#9220)) **Bug fixes** - Fixed icon size in `EuiSuperDatePicker`'s time window buttons when `compressed={true}` ([#9245](elastic/eui#9245)) - Fixed `EuiIcon` visibility issue with `logoElastic` when `color` is set to `text` or `ghost` in light mode ([#9247](elastic/eui#9247)) - Fixed `EuiInMemoryTable` support for controlled search for plain text (when `searchFormat="text"`) by properly handling `search.query` and `search.defaulQuery` ([#9142](elastic/eui#9142)) **Breaking changes** - Removed deprecated `message` prop from `EuiInMemoryTable`, use `noItemsMessage` instead ([#9234](elastic/eui#9234)) **Accessibility** - Improved the accessibility of input fields in the popover of `EuiSuperDatePicker` by properly labeling them ([#9239](elastic/eui#9239)) - Improved the accessibility of `EuiSelectable` by removing empty `aria-activedescendant` attribute when no option is active to ensure the search input is perceivable by screen readers ([#9223](elastic/eui#9223)) --------- Co-authored-by: Weronika Olejniczak <[email protected]> Co-authored-by: Jorge Oliveira <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
## Dependency updates - `@elastic/eui`: `v109.2.0` ⏩ `v110.0.0` --- ## Changes - Updates usages of the `message` prop on `EuiInMemoryTable` to `noItemsMessage` - Updates usages of `EuiSearchBar`'s `onChange` prop to check for the `query` argument, as it may be `null` now - Removes obsolete `color` props on implementations of `EuiPageTemplate` and `EuiPageTemplate.Header` (`color` prop wasn't applied before either) ## Package updates ### `@elastic/eui` v110.0.0 - Updated `EuiSuperDatePicker` to expose plain text `utcOffset` and `timeZoneName` in `timeZoneDisplayProps.customRender` render function ([elastic#9245](elastic/eui#9245)) - Updated `EuiHeaderLogo` to add a hover style ([elastic#9240](elastic/eui#9240)) - Made `EuiQuickSelectPanel` available for importing from the `@elastic/eui` package ([elastic#9239](elastic/eui#9239)) - Updated `EuiAvatar` to support Emoji Sequence ("advanced") in the `initials` prop ([elastic#9227](elastic/eui#9227)) - Added `color` prop on `EuiPageHeader` to support `transparent` (default) and `plain` backgrounds. ([elastic#9220](elastic/eui#9220)) - Added `color` prop on `EuiPage` to support `transparent` (default) and `plain` backgrounds. ([elastic#9220](elastic/eui#9220)) - Updated `EuiPageTemplate` to ensure `panelled=true` renders a `EuiPageHeader` with a plain background by default ([elastic#9220](elastic/eui#9220)) - Removed the default background style on `EuiPageTemplate`'s outer wrapper ([elastic#9220](elastic/eui#9220)) **Bug fixes** - Fixed icon size in `EuiSuperDatePicker`'s time window buttons when `compressed={true}` ([elastic#9245](elastic/eui#9245)) - Fixed `EuiIcon` visibility issue with `logoElastic` when `color` is set to `text` or `ghost` in light mode ([elastic#9247](elastic/eui#9247)) - Fixed `EuiInMemoryTable` support for controlled search for plain text (when `searchFormat="text"`) by properly handling `search.query` and `search.defaulQuery` ([elastic#9142](elastic/eui#9142)) **Breaking changes** - Removed deprecated `message` prop from `EuiInMemoryTable`, use `noItemsMessage` instead ([elastic#9234](elastic/eui#9234)) **Accessibility** - Improved the accessibility of input fields in the popover of `EuiSuperDatePicker` by properly labeling them ([elastic#9239](elastic/eui#9239)) - Improved the accessibility of `EuiSelectable` by removing empty `aria-activedescendant` attribute when no option is active to ensure the search input is perceivable by screen readers ([elastic#9223](elastic/eui#9223)) --------- Co-authored-by: Weronika Olejniczak <[email protected]> Co-authored-by: Jorge Oliveira <[email protected]> Co-authored-by: Elastic Machine <[email protected]>
Summary
Closes #9019 (see #9019 (comment))
This addresses 2 issues with
EuiInMemoryTablewhen used with plain text, namelysearchFormat="text", it:search.querypropsearch.defaultQueryis being passed to the search boxIt's a follow-up for a previous fix in #9059
Why are we making this change?
To fix a bug, and to unblock fixing another bug in Kibana.
Impact to users
🟢 Since this is a bug fix impact should just be positive, but tests may fail expecting the old broken behavior. However, no instances of
searchFormat="text"can be found in Kibana with a simple search. And these changes should not affect the defaultsearchFormat="eql".QA
Suggestion
The bug happens when the component attempts to parse the search value as EQL syntax despite
searchFormatbeingtext, although it shouldn't, as expressed in the description of thesearchFormatprop.To reproduce it, you can:
mainorin the search box should throw an error and breaksearchTextistextand the controlled logic is correctsearch.defaultQueryworks as expected, in the Storybook UI you can set thesearchprop to{ query: null, defaultQuery: 'hello world' }This test story should work as expected in this branch.
General checklist
Checked in both light and dark modesChecked in both MacOS and Windows high contrast modes(emulate forced colors if you do not have access to a Windows machine.)Checked in mobileChecked in Chrome, Safari, Edge, and FirefoxChecked for accessibility including keyboard-only and screenreader modesAdded documentation@defaultif default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesUpdated visual regression testsIf applicable, added the breaking change issue label (and filled out the breaking change checklist)If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)