-
Notifications
You must be signed in to change notification settings - Fork 843
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
[Docs] Replace Static HTML Element IDs with htmlIdGenerator()()
#5114
Conversation
…Ds generated by the htmlIdGenerator utility function
…generated by the htmlIdGenerator utility function
…generated by the htmlIdGenerator utility function
…enerated by the htmlIdGenerator utility function
…generated by the htmlIdGenerator utility function
… to use IDs generated by the htmlIdGenerator utility function
…generated by the htmlIdGenerator utility function
…e IDs generated by the htmlIdGenerator utility function
…IDs generated by the htmlIdGenerator utility function
…se IDs generated by the htmlIdGenerator utility function
…e IDs generated by the htmlIdGenerator utility function
…o use IDs generated by the htmlIdGenerator utility function
…IDs generated by the htmlIdGenerator utility function
…se IDs generated by the htmlIdGenerator utility function
Preview documentation changes for this PR: https://eui.elastic.co/pr_5114/ |
…e IDs generated by the htmlIdGenerator utility function
Preview documentation changes for this PR: https://eui.elastic.co/pr_5114/ |
🙌 this is great! A couple notes from the team sync discussion that push this further: ID variable namingIt was proposed that we look into a naming convention for these ID variables. The existing documentation guidelines wiki seems like a good place to document this. Provide additional distinctions to the ID generatorWe should explore passing a value to the id generation to further differentiate usages. It probably makes sense in some places and not others, is there a common way to identify these and standardize? b2ba60a#diff-9c7ad923324bb4de8dc9f3190e7c0e61a86552db898ace382af0f17ac2829755R81
could become
Create a hook to prevent IDs from changing between rendersLess useful in practice for small instances, but impactful if a component or its usage has a lot of generated IDs, we should cache the generated IDs between renders. The first idea was to use a useState or useRef to do so, but to keep this more readable and less boiler-platey there was an additional suggestion of creating a |
@constancecchen has started working on a useHtmlGeneratorId hook that can be used in the docs as well. Once PR #5133 is merged, I'll update the code here. We're still open to opinions on the ID variable naming and providing distinctions for the generator function. |
Variable namingMy vote is for DistinctionsPrimarily for letting human eyes distinguish between these IDs, they are useful in labelling related IDs. https://github.com/elastic/eui/pull/5114/files#diff-5b897b5fbf788b47fa1abebfbfb95f050a2e2f134eb609dccb8167ed3276c423R63 looks like a good case, where the suffix could be used to distinguish between the flyout and title IDs. |
Closing this PR because I created four smaller requests to handle this issue. The PRs that were merged to resolve this issue are:
|
Summary
Updates any static HTML element IDs inside of EUI documentation with IDs generated by the
htmlIdGenerator()()
utility function. This change is based on Issue #4774 where it was reported that including multiple copies of the same code snippet can cause duplicate ID errors. While static IDs were removed, the existing generated IDs were standardized across relevant files.Page-by-page review of the docs revealed that there are about 80 instances of HTML element IDs in the docs. Here is a document defining the pages and components affected by this change.
Before

After

Checklist