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

[EuiHighlight] Support passing an array of search strings + performance improvements #7496

Merged
merged 11 commits into from
Feb 5, 2024

Conversation

cee-chen
Copy link
Member

@cee-chen cee-chen commented Jan 31, 2024

Summary

closes #5798

This PR:

  • Implements search: string | string[] logic, allowing multiple separate search values to be passed
  • Fixes the crashing bug reported in [EuiHighlight] Highlight separated words #5798 by correctly escaping regex characters 😅
  • Implements many performance/developer readability improvements on the component, splitting logic up into memoized subcomponents
  • Improves unit tests

⚠️ There's a lot of cleanup and performance improvements in this PR, so as always, I strongly recommend following by commit!

Please note: the actual splitting of strings into an array from the user input needs to be managed by the consumer, and not by EUI. This gives consuming devs full flexibility for, e.g. the kind of delimiter to use, when and when not to highlight separate words, etc.

QA

Bugfix testing - regex escaping

Feature testing

  • Go to https://eui.elastic.co/pr_7496/#/utilities/highlight-and-mark#highlight
  • Toggle the Highlight all switch
  • Toggle the Search for an array of separate words switch
  • Confirm that jumped and over are separate highlights
  • Type in two words into the search that are not adjacent, e.g. quick lazy
  • Confirm that both words are highlighted separately (see above screenshot)

General checklist

  • Browser QA
    - [ ] Checked in both light and dark modes
    - [ ] Checked in mobile
    • Checked in Chrome, Safari, Edge, and Firefox
    • Checked for accessibility including keyboard-only and screenreader modes
  • Docs site QA
  • Code quality checklist
  • Release checklist
    • A changelog entry exists and is marked appropriately.
      - [ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)
  • Designer checklist - N/A

- allows us to avoid a while loop and prevents extra crashes
- only if `highlightAll` is also passed, doesn't really make sense otherwise
@cee-chen cee-chen changed the title [EuiHighlight] Support passing an array of search strings [EuiHighlight] Support passing an array of search strings + performance improvements Jan 31, 2024
- add toggle with array behavior (needs to be controlled/set by the consumer, not EUI - this allows us to not have to deal with a delimiter, filtering, etc)

+ convert examples to TSX while here

+ Fix playground error
- remove snapshots and prefer assertions instead

- move `hasScreenReaderHelpText` test out of incorrect describe block
there's basically 2 different components/separate sets of logic going on here, so we might as well represent them as such, which:

1. takes better advantage of React memoization

2. is easier to read/parse standalone
- let it be memoized/the concern of the parent `EuiHighlight` instead, and allow the subcomponent to flexibly render whatever (defaulting to an unstyled `<mark>` if nothing is passed)
- makes it easier to debug code in React Devtools' Components tab

+ update our lint rule that we set to `.forwardRef` to also apply to `.memo()`ed components - will matter more as we start to optimize performance across EUI
- FunctionComponent wants a JSX return, not a string
@cee-chen cee-chen marked this pull request as ready for review January 31, 2024 01:21
@cee-chen cee-chen requested a review from a team as a code owner January 31, 2024 01:21
@tkajtoch tkajtoch self-requested a review January 31, 2024 15:56
Copy link
Member

@tkajtoch tkajtoch left a comment

Choose a reason for hiding this comment

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

I love the eslint plugin update!

LGTM

@cee-chen
Copy link
Member Author

cee-chen commented Feb 1, 2024

There's been some memory concerns about the performance improvements mentioned offline - going to hold off on merging this PR until we've sorted them out!

@cee-chen cee-chen enabled auto-merge (squash) February 5, 2024 19:08
@kibanamachine
Copy link

Preview staging links for this PR:

@cee-chen cee-chen merged commit dc604af into elastic:main Feb 5, 2024
6 of 7 checks passed
@cee-chen cee-chen deleted the highlight-multiple branch February 5, 2024 19:35
@elasticmachine
Copy link
Collaborator

💚 Build Succeeded

History

cee-chen added a commit to elastic/kibana that referenced this pull request Feb 20, 2024
`v93.0.0` ⏩ `v93.1.1`

---

## [`v93.1.1`](https://github.com/elastic/eui/releases/v93.2.0)

**This is a patch release primarily intended for use by Kibana.**

- Added top-level `EuiTreeView.Item` export
([#7526](elastic/eui#7526))

## [`v93.1.0`](https://github.com/elastic/eui/releases/v93.1.0)

- Added `index` glyph to `EuiIcon`
([#7498](elastic/eui#7498))
- Updated `EuiHighlight` to accept an array of `search` strings, which
allows highlighting multiple, separate words within its children. This
new type and behavior *only* works if `highlightAll` is also set to
true. ([#7496](elastic/eui#7496))
- Updated `EuiContextMenu` with a new `panels.items.renderItem`
property, which allows rendering completely custom items next to
standard `EuiContextMenuItem` objects
([#7510](elastic/eui#7510))
- `EuiSuperDatePicker` updates:
- Updated `EuiSuperDatePicker` with a new `refreshIntervalUnits` prop.
Passing this prop allows controlling and overriding the default unit
rounding behavior. ([#7501](elastic/eui#7501))
- Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new
`intervalUnits` prop. Passing this prop allows controlling and
overriding the default unit rounding behavior.
([#7501](elastic/eui#7501))
- Updated `onRefreshChange` to pass back a new `intervalUnits` key that
contains the current interval unit format (seconds, minutes, or hours).
([#7501](elastic/eui#7501))
- Updated `EuiSuperDatePicker` with a new `canRoundRelativeUnits` prop,
which defaults to true (current behavior). To preserve displaying the
unit that users select for relative time, set this to false.
([#7502](elastic/eui#7502))
- Updated `EuiSuperDatePicker` with a new `refreshMinInterval` prop,
which accepts a minimum number in milliseconds
([#7516](elastic/eui#7516))
- Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new
`minInterval` prop, which accepts a minimum number in milliseconds
([#7516](elastic/eui#7516))

**Bug fixes**

- Fixed `EuiHighlight` to not parse `search` strings as regexes
([#7496](elastic/eui#7496))
- Fixed `EuiSuperDatePicker` submit bug when used within `<form>`
elements ([#7504](elastic/eui#7504))
- Fixed an `EuiTreeView` bug where `aria-expanded` was being applied to
items without expandable children
([#7513](elastic/eui#7513))

**CSS-in-JS conversions**

- Converted `EuiTreeView` to Emotion. Updates as part of the conversion:
([#7513](elastic/eui#7513))
  - Removed `.euiTreeView__wrapper` div node
  - Enforced consistent `icon` size based on `display` size
fkanout pushed a commit to fkanout/kibana that referenced this pull request Mar 4, 2024
`v93.0.0` ⏩ `v93.1.1`

---

## [`v93.1.1`](https://github.com/elastic/eui/releases/v93.2.0)

**This is a patch release primarily intended for use by Kibana.**

- Added top-level `EuiTreeView.Item` export
([elastic#7526](elastic/eui#7526))

## [`v93.1.0`](https://github.com/elastic/eui/releases/v93.1.0)

- Added `index` glyph to `EuiIcon`
([elastic#7498](elastic/eui#7498))
- Updated `EuiHighlight` to accept an array of `search` strings, which
allows highlighting multiple, separate words within its children. This
new type and behavior *only* works if `highlightAll` is also set to
true. ([elastic#7496](elastic/eui#7496))
- Updated `EuiContextMenu` with a new `panels.items.renderItem`
property, which allows rendering completely custom items next to
standard `EuiContextMenuItem` objects
([elastic#7510](elastic/eui#7510))
- `EuiSuperDatePicker` updates:
- Updated `EuiSuperDatePicker` with a new `refreshIntervalUnits` prop.
Passing this prop allows controlling and overriding the default unit
rounding behavior. ([elastic#7501](elastic/eui#7501))
- Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new
`intervalUnits` prop. Passing this prop allows controlling and
overriding the default unit rounding behavior.
([elastic#7501](elastic/eui#7501))
- Updated `onRefreshChange` to pass back a new `intervalUnits` key that
contains the current interval unit format (seconds, minutes, or hours).
([elastic#7501](elastic/eui#7501))
- Updated `EuiSuperDatePicker` with a new `canRoundRelativeUnits` prop,
which defaults to true (current behavior). To preserve displaying the
unit that users select for relative time, set this to false.
([elastic#7502](elastic/eui#7502))
- Updated `EuiSuperDatePicker` with a new `refreshMinInterval` prop,
which accepts a minimum number in milliseconds
([elastic#7516](elastic/eui#7516))
- Updated `EuiAutoRefresh` and `EuiRefreshInterval` with a new
`minInterval` prop, which accepts a minimum number in milliseconds
([elastic#7516](elastic/eui#7516))

**Bug fixes**

- Fixed `EuiHighlight` to not parse `search` strings as regexes
([elastic#7496](elastic/eui#7496))
- Fixed `EuiSuperDatePicker` submit bug when used within `<form>`
elements ([elastic#7504](elastic/eui#7504))
- Fixed an `EuiTreeView` bug where `aria-expanded` was being applied to
items without expandable children
([elastic#7513](elastic/eui#7513))

**CSS-in-JS conversions**

- Converted `EuiTreeView` to Emotion. Updates as part of the conversion:
([elastic#7513](elastic/eui#7513))
  - Removed `.euiTreeView__wrapper` div node
  - Enforced consistent `icon` size based on `display` size
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.

[EuiHighlight] Highlight separated words
4 participants