-
Notifications
You must be signed in to change notification settings - Fork 842
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] Allow consumers to use non-EQL plain text search with special characters #7175
[EuiInMemoryTable] Allow consumers to use non-EQL plain text search with special characters #7175
Conversation
- not clear why it lives in `EuiSearchBox`, which doesn't reference it whatsoever + clean up/combine `EuiInMemoryTable` imports
4634839
to
7d33a52
Compare
7d33a52
to
eaa1ea5
Compare
+ update faker.js to latest to get string.symbol API
eaa1ea5
to
4d8fd29
Compare
* such as quotes, parentheses, and colons, which are normally otherwise | ||
* reserved for EQL syntax. | ||
*/ | ||
searchPlainText?: boolean; |
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'm not 100% convinced this PR is the right way to go, so I'm opening it to get earlier thoughts from the rest of the team.
-
I don't love this prop name or the "plain text" terminology I'm using - I'm open to other ways to describe "text that does not follow EQL filtering syntax and thus allows users to search for special characters"
-
I experimented with different ways to pass this flag in the
search
object, e.g.<EuiInMemoryTable search={{ plainText: true }} />
, but it ended up causing a bunch of type headaches down the line so I abandoned it in favor of a top-level prop. Let me know if you have thoughts/preferences on this
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.
Just thinking out loud, what about searchInputFormat: 'eql' | 'text'
or just searchFormat
?
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 love it! I'll go with searchFormat
!
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.
searchFormat
prop update: feb7a91
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.
Also, leaving some extra thoughts just for more context, in case future devs come by this PR and are like "why did they do things this way":
I'm not 100% convinced this PR is the right way to go
When I mentioned this above, I was referring to the fact that this "plain text" search is still essentially using EQL under the hood, primarily to execute over every item
passed to the memory table. This potentially isn't as performant as it could be if we were using a more raw EuiFieldSearch
input and skipping using EuiSearchBar.Query.parse
entirely.
However, in the end, I think this is a simpler approach to go with because while skipping EQL entirely might be more performant, this at least isn't less performant than current behavior, and additionally has much less dev overhead/testing required (we'd have to write a brand new utility to execute/iterate over every single item otherwise).
In the future, something might change to necessitate a bigger break between EQL and non EQL search - but in the interim, I think this is an okay compromise between performance and developer time spent.
|
||
**Bug fixes** | ||
|
||
- Fixed missing i18n in `EuiSearchBar`'s default placeholder and aria-label text |
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.
Not sure how this got missed for over 4 years 🥲
@@ -127,7 +127,7 @@ | |||
"@emotion/eslint-plugin": "^11.11.0", | |||
"@emotion/jest": "^11.11.0", | |||
"@emotion/react": "^11.11.0", | |||
"@faker-js/faker": "^7.6.0", | |||
"@faker-js/faker": "^8.0.2", |
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.
Upgrade needed for the new faker.string.symbol
API I'm using to generate random special characters. Probably safe to do now that the faker controversy is behind us 🙈
I'm really happy with the changes in this PR. I'd like to get the regex not throwing errors when typing |
Preview staging links for this PR:
|
💚 Build Succeeded
History
|
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.
LGTM 🚢
`v88.3.0`⏩`v88.5.0` closes #151514 --- ## [`88.5.0`](https://github.com/elastic/eui/tree/v88.5.0) - Updated `EuiCallOut` with a new `onDismiss` prop ([#7156](elastic/eui#7156)) - Added a new `renderCustomToolbar` prop to `EuiDataGrid`, which allows custom rendering of the toolbar. ([#7190](elastic/eui#7190)) - Added a new `allowResetButton` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows hiding the "Reset to default" button from the display settings popover. ([#7190](elastic/eui#7190)) - Added a new `additionalDisplaySettings` prop to `toolbarVisibility.showDisplaySelector` of `EuiDataGrid`, which allows rendering extra settings inside the display settings popover. ([#7190](elastic/eui#7190)) - Updated `EuiDataGrid`'s toolbar display settings button icon ([#7190](elastic/eui#7190)) - Updated `EuiTextTruncate` with significantly improved iteration performance. Removed `measurementRenderAPI` prop, as `EuiTextTruncation` now only uses more performant canvas render API ([#7210](elastic/eui#7210)) - Updated `EuiPopover` with a new configurable `repositionToCrossAxis` prop ([#7211](elastic/eui#7211)) - Updated `EuiDatePicker` to support `compressed` input styling ([#7218](elastic/eui#7218)) - Added `gradient` and `palette` icon glyphs. ([#7220](elastic/eui#7220)) **Bug fixes** - Fixed `EuiPopover`'s missing animations on popover close ([#7211](elastic/eui#7211)) - Fixed `EuiInputPopover` anchoring to the wrong side and missing shadows on smaller screens ([#7211](elastic/eui#7211)) - Fixed `EuiSuperDatePicker` icon spacing on the quick select button ([#7217](elastic/eui#7217)) - Fixed a missing type in `EuiMarkdownEditor`'s default processing plugins ([#7221](elastic/eui#7221)) ## [`88.4.1`](https://github.com/elastic/eui/tree/v88.4.1) **Bug fixes** - Fixed missing `className`s on `EuiTextTruncate` ([#7212](elastic/eui#7212)) - Fixed `title`s on `EuiComboBox` dropdown options to always be present ([#7212](elastic/eui#7212)) - Fixed `EuiComboBox` truncation issues when search is an empty space ([#7212](elastic/eui#7212)) ## [`88.4.0`](https://github.com/elastic/eui/tree/v88.4.0) - Updated `EuiComboBox` to allow configuring text truncation behavior via `truncationProps`. These props can be set on the entire combobox as well as on on individual dropdown options. ([#7028](elastic/eui#7028)) - Updated `EuiInMemoryTable` with a new `searchFormat` prop (defaults to `eql`). When setting this prop to `text`, the built-in search bar will ignore EQL syntax and allow searching for plain strings with special characters and symbols. ([#7175](elastic/eui#7175)) **Bug fixes** - `EuiComboBox` now always shows the highlighted search text, even on truncated text ([#7028](elastic/eui#7028)) - Fixed missing i18n in `EuiSearchBar`'s default placeholder and aria-label text ([#7175](elastic/eui#7175)) - Fixed the inline compressed styles of `EuiDescriptionListTitle` to use a taller line-height for readability ([#7185](elastic/eui#7185)) - Fixed `EuiComboBox` to correctly truncate selected items when displayed as pills and plain text ([#7193](elastic/eui#7193)) **Accessibility** - Added `aria-current` attribute to `EuiTablePagination` ([#7186](elastic/eui#7186)) **CSS-in-JS conversions** - Converted `EuiDroppable` and `EuiDraggable` to Emotion; Removed `$euiDragAndDropSpacing` Sass variables ([#7187](elastic/eui#7187)) --------- Co-authored-by: Patryk Kopycinski <[email protected]> Co-authored-by: Jan Monschke <[email protected]> Co-authored-by: Thomas Watson <[email protected]>
Summary
closes #7160
This PR contains some incidental cleanup - please follow along by commit. 65e5205 is the main feature/fix.
Before (see error icon/message)
After
QA
Incremental
switch for faster feedback, and then thePlain text search
switchGeneral checklist
@default
if default values are missing)and playground togglesand cypress tests- [ ] Checked in both light and dark modes- [ ] Checked in mobile- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for breaking changes and labeled appropriately- [ ] Checked for accessibility including keyboard-only and screenreader modes- [ ] Updated the Figma library counterpart