Add useMemo util for htmlIdGenerator + update EuiDataGrid's IDs to not change on every rerender #5133

cee-chen · 2021-09-02T15:59:21Z

Summary

@kqualters-elastic recently raised that our EuiDataGrid IDs were regenerating on every change/rerender (e.g. click/sort/paginate events, etc.). The way to fix this is to memoize the generated ID.

I opted to create a custom hook helper with useRef, as the generated ID isn't really meant to change unless the component fully unmounts/mounts, and I think useRef conveys that slightly more accurately than state.

Per @chandlerprall's excellent breakdown and @breehall's work, I've determined that useMemo definitely beats out useRef in terms of usage and updated this PR accordingly!

Before

(Note the yellow flash on the id props on change)

After

Checklist

~~- [ ] Check against all themes for compatibility 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 the any docs examples
Added or updated jest tests

~~- [ ] Checked for breaking changes and labeled appropriately~~

Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

cee-chen · 2021-09-02T16:02:46Z

As a note, I only updated EuiDataGrid for now to get early comments out of the way, but I'm planning on doing a pass on all components with rerendering generated IDs. FWIW It's not all components using htmlIdGenerator, as some of them are either using useState/useRef already, or are class components that store the generated ID on init which also doesn't rerender. Do y'all have any preference as to whether those updates should be a separate PR, or if I should continue extending this PR?

src/services/accessibility/html_id_generator.ts

kibanamachine · 2021-09-02T16:24:42Z

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

- that creates a memoized ID, preventing the issue of re-randomized IDs on every component change

…props) + update EuiCollapsibleNav as an example of real-world usage

cee-chen · 2021-09-03T00:34:21Z

👋 Hey y'all! I ended up squashing in my changes in a rebase, since there were many of them after our recent team discussion.

The initial setup (c5bd90b) now uses useMemo instead of useRef, and is much awesomer for it! Thanks Chandler and Bree!
- I also renamed the fn to differentiate it more clearly from the original generator, and renamed the params from idPrefix->prefix for brevity
I added 5921ac7 which adds a 3rd idFromProps param, and an example conversion within EuiCollapsibleNav. I'm not super in love with the naming of this one though, does anyone have any ideas? Here's the options I tossed around, not sure if they're any better:
- idFromProps
- customId
- overrideId
I added docs in 1009e4c and would super appreciate feedback there!! It'll be my first time officially adding copy/docs examples and I'm not totally sure I nailed it 😅

Thanks y'all! Locally, I'll keep beavering away at converting more src/components/ to the new util!

kibanamachine · 2021-09-03T00:52:23Z

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

cee-chen · 2021-09-07T20:15:21Z

👋 Re-pinging for review since this is potentially blocking @breehall's PR!

cchaos

I mainly just looked at the new hook and it's associated docs. Had a couple of suggestions.

src-docs/src/views/html_id_generator/html_id_generator_example.js

src/services/accessibility/html_id_generator.ts

src-docs/src/views/html_id_generator/html_id_generator_example.js

src-docs/src/views/html_id_generator/use_generated_html_id.js

src-docs/src/views/html_id_generator/html_id_generator_example.js

kibanamachine · 2021-09-07T20:32:57Z

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

Co-authored-by: Caroline Horn <[email protected]>

kibanamachine · 2021-09-07T21:57:37Z

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

kibanamachine · 2021-09-07T22:41:25Z

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

+ include more documentation text

…ratedHtmlId

kibanamachine · 2021-09-09T01:58:08Z

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

thompsongl

Love how simple this hook turned out to be!

src/services/accessibility/html_id_generator.ts

src/services/accessibility/html_id_generator.testenv.ts

cchaos

👍 LGTM, only checked in Chrome though and only the Docs of the new hook.

src-docs/src/views/html_id_generator/html_id_generator_reuse.js

- update comment from ref to memo - Skip EuiFlexGroup

cee-chen · 2021-09-09T15:49:06Z

I just did another quick QA pass and noticed all but 1 of the CodeSandbox links on the HTML ID generator demos are broken because they export named components instead of a default function 🙈 I'll fix that here shortly!

kibanamachine · 2021-09-09T16:00:19Z

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

- Changed exports from named exports to default in order for demos to work in CodeSandbox - Added `htmlIdGenerator` namespacing for clarity to all prefix/suffix sections - Fixed casing on `bothPrefixSuffix.js` to snake_casing (per rest of codebase) - Fixed casing of all source & snippet vars to camelCasing - Fixed `sufix` typo

kibanamachine · 2021-09-09T16:28:05Z

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

thompsongl

LGTM!
Codesandbox links are working, except for the one with the new hook, which is to be expected before it's released.

cee-chen · 2021-09-09T16:36:53Z

Ahh phew, thanks a million Greg, I was worried I'd done something wrong there! 😅

Merging this in now!

thompsongl · 2021-09-09T16:40:40Z

Ahh phew, thanks a million Greg, I was worried I'd done something wrong there!

Yeah we currently don't have a method to use a pre-release EUI package in codesandbox, so it uses the latest release. So any new things components, services, or props in a PR are unavailable.

cee-chen commented Sep 2, 2021

View reviewed changes

src/services/accessibility/html_id_generator.ts Outdated Show resolved Hide resolved

breehall mentioned this pull request Sep 2, 2021

[Docs] Replace Static HTML Element IDs with htmlIdGenerator()() #5114

Closed

10 tasks

cee-chen added 4 commits September 2, 2021 16:47

Create a htmlIdGenerator utility hook

c5bd90b

- that creates a memoized ID, preventing the issue of re-randomized IDs on every component change

Update test env mocks with new helper

6e51759

Update EuiDataGrid with static IDs

f5c3c39

Add API for overriding generated IDs with custom IDs (primarily from …

5921ac7

…props) + update EuiCollapsibleNav as an example of real-world usage

cee-chen changed the title ~~Add useRef util for htmlIdGenerator + update EuiDataGrid's IDs to not change on every rerender~~ Add useMemo util for htmlIdGenerator + update EuiDataGrid's IDs to not change on every rerender Sep 2, 2021

Add changelog entry and documentation

1009e4c

cee-chen force-pushed the use-htmlidgenerator branch from 9e8f013 to 1009e4c Compare September 3, 2021 00:26

Merge branch 'master' into use-htmlidgenerator

04a1832

thompsongl self-requested a review September 7, 2021 20:16

cchaos reviewed Sep 7, 2021

View reviewed changes

PR documentation feedback

0a2a20a

Co-authored-by: Caroline Horn <[email protected]>

cee-chen force-pushed the use-htmlidgenerator branch from 704c3c8 to 0a2a20a Compare September 7, 2021 21:32

Merge branch 'master' into use-htmlidgenerator

0bafa10

cee-chen added 4 commits September 8, 2021 14:31

Merge branch 'master' into use-htmlidgenerator

414f0be

[PR feedback] Change idFromProps to conditionalId

0e7325a

Document useGeneratedHtmlId options in a props table

61f6ab8

+ include more documentation text

More clearly document distinction between htmlIdGenerator and useGene…

e5049fa

…ratedHtmlId

thompsongl reviewed Sep 9, 2021

View reviewed changes

src/services/accessibility/html_id_generator.ts Outdated Show resolved Hide resolved

src/services/accessibility/html_id_generator.testenv.ts Show resolved Hide resolved

cchaos approved these changes Sep 9, 2021

View reviewed changes

src-docs/src/views/html_id_generator/html_id_generator_reuse.js Outdated Show resolved Hide resolved

cee-chen added 3 commits September 9, 2021 08:32

Address PR feedback

1317d94

- update comment from ref to memo - Skip EuiFlexGroup

Merge branch 'master' into use-htmlidgenerator

5a5b167

Spotted a typo

09142a8

thompsongl approved these changes Sep 9, 2021

View reviewed changes

cee-chen merged commit eca5d49 into elastic:master Sep 9, 2021

cee-chen deleted the use-htmlidgenerator branch September 9, 2021 16:37

1Copenut mentioned this pull request Sep 16, 2021

Bumping EUI to 38.0.1 elastic/kibana#112462

Merged

This was referenced Sep 16, 2021

Update multiple functional components to useGeneratedHtmlId hook #5195

Merged

[EuiBasicTable] Fix generated IDs regenerating on rerender #5196

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add useMemo util for htmlIdGenerator + update EuiDataGrid's IDs to not change on every rerender #5133

Add useMemo util for htmlIdGenerator + update EuiDataGrid's IDs to not change on every rerender #5133

cee-chen commented Sep 2, 2021 •

edited

Loading

cee-chen commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

cee-chen commented Sep 3, 2021

kibanamachine commented Sep 3, 2021

cee-chen commented Sep 7, 2021

cchaos left a comment

kibanamachine commented Sep 7, 2021

kibanamachine commented Sep 7, 2021

kibanamachine commented Sep 7, 2021

kibanamachine commented Sep 9, 2021

thompsongl left a comment

cchaos left a comment

cee-chen commented Sep 9, 2021 •

edited

Loading

kibanamachine commented Sep 9, 2021

kibanamachine commented Sep 9, 2021

thompsongl left a comment

cee-chen commented Sep 9, 2021

thompsongl commented Sep 9, 2021 •

edited

Loading

Add useMemo util for htmlIdGenerator + update EuiDataGrid's IDs to not change on every rerender #5133

Add useMemo util for htmlIdGenerator + update EuiDataGrid's IDs to not change on every rerender #5133

Conversation

cee-chen commented Sep 2, 2021 • edited Loading

Summary

Before

After

Checklist

cee-chen commented Sep 2, 2021

kibanamachine commented Sep 2, 2021

cee-chen commented Sep 3, 2021

kibanamachine commented Sep 3, 2021

cee-chen commented Sep 7, 2021

cchaos left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 7, 2021

kibanamachine commented Sep 7, 2021

kibanamachine commented Sep 7, 2021

kibanamachine commented Sep 9, 2021

thompsongl left a comment

Choose a reason for hiding this comment

cchaos left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 9, 2021 • edited Loading

kibanamachine commented Sep 9, 2021

kibanamachine commented Sep 9, 2021

thompsongl left a comment

Choose a reason for hiding this comment

cee-chen commented Sep 9, 2021

thompsongl commented Sep 9, 2021 • edited Loading

cee-chen commented Sep 2, 2021 •

edited

Loading

cee-chen commented Sep 9, 2021 •

edited

Loading

thompsongl commented Sep 9, 2021 •

edited

Loading