-
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
feat: update useGeneratedHtmlId to use React.useId when available #7095
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_7095/ |
75f875e
to
7c76542
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7095/ |
1 similar comment
Preview documentation changes for this PR: https://eui.elastic.co/pr_7095/ |
// pageInnerId may contain colons that are parsed as pseudo-elements if not escaped | ||
parent: `#${CSS.escape(pageInnerId)}`, |
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.
TIL there's a global CSS
var, and TIL CSS.escape 🤯
@tkajtoch just to confirm - does this global exist on SSR as well as in browsers?
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.
it's available in all modern browsers (caniuse) and Next.js client components. Other SSR environments may or may not have it, but usually they're using JSDOM polyfills for this and other very common properties like HTMLElement
or various observers.
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.
Sounds good to me, Next.js will likely be our biggest SSR framework in terms of usage in any case.
Not a change request, but if this comes up in the future from a consumer we could also consider a basic regex like .replace(/:/g, '\\:')
instead if colons are the only issue here - I would consider that a lesser lift on our part than consumers needing a polyfill.
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
Co-authored-by: Cee Chen <549407+cee-chen@users.noreply.github.com>
983da16
to
7e59b17
Compare
Preview documentation changes for this PR: https://eui.elastic.co/pr_7095/ |
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.
Tests look fantastic - thanks so much! I now have a much clearer idea of what useId
does/looks like
Preview documentation changes for this PR: https://eui.elastic.co/pr_7095/ |
💚 Build Succeeded
History
cc @tkajtoch |
`87.1.0` ➡️ `87.2.0` ## [`87.2.0`](https://github.com/elastic/eui/tree/v87.2.0) - `EuiResizableButton` is now available as a generic top-level export ([#7087](elastic/eui#7087)) - Added new `alignIndicator` prop to `EuiResizableButton`. Defaults to `center`, and can now additionally be configured to `start` and `end` ([#7087](elastic/eui#7087)) - Updated `useGeneratedHtmlId` hook to use `React.useId` as the source of unique identifiers when available ([#7095](elastic/eui#7095)) **CSS-in-JS conversions** - Converted `EuiResizableButton` to Emotion; Removed `$euiResizableButtonTransitionSpeed` and `$euiResizableButtonSize` ([#7081](elastic/eui#7081)) - Converted `EuiResizableCollapseButton` to Emotion ([#7091](elastic/eui#7091)) --------- Co-authored-by: Cee Chen <constance.chen@elastic.co>
Summary
This PR resolves #7078 by updating
useGeneratedHtmlId
utility function to useReact.useId
when it's available (which means when running React 18).It's the first step in making
useGeneratedHtmlId
and its usages SSR-compatible. We must do #7094 to makeuseGeneratedHtmlId
SSR-safe across all supported React versions.QA
<EuiSwitch>
and<EuiDataGrid>
generated DOMid
attributes reference the same objects as before. The id values will be different (instead of UUIDs there's gonna be strings like:r1:
generated byReact.useId
)General checklist