Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 677
perf(ActionList): replace :has() selector with JS-computed attribute#7714
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
Uh oh!
There was an error while loading. Please reload this page.
Changes from all commits
a0eb6fd0919c3bcb1dc51849232e0f6fd8f06729b364dbb7b1538c916bc82ff05168dba90f58ab4fb915abb7503File filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@primer/react": patch | ||
| --- | ||
| perf(ActionList): replace :has() selector with JS-computed attribute |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -9,6 +9,7 @@ import {useProvidedRefOrCreate} from '../hooks' | ||
| import {FocusKeys, useFocusZone} from '../hooks/useFocusZone' | ||
| import {clsx} from 'clsx' | ||
| import classes from './ActionList.module.css' | ||
| import useIsomorphicLayoutEffect from '../utils/useIsomorphicLayoutEffect' | ||
| const UnwrappedList = <As extends React.ElementType = 'ul'>( | ||
| props: ActionListProps<As>, | ||
| @@ -66,6 +67,34 @@ const UnwrappedList = <As extends React.ElementType = 'ul'>( | ||
| [variant, selectionVariant, containerSelectionVariant, showDividers, listRole, headingId], | ||
| ) | ||
| // Replaces a CSS `:has([data-has-description])` selector that caused full-subtree | ||
| // style recalculation on every DOM mutation (~674ms on 100 items, 10-20s freezes on Safari). | ||
| // | ||
| // Ideally we'd derive this from children during render, but each Item's description is | ||
| // detected via `useSlots` at render time, so the List can't know which Items have | ||
| // descriptions without duplicating slot detection or deeply inspecting children trees | ||
| // (fragile with Groups, conditional rendering, wrapper components, etc.). | ||
| // | ||
| // A context-based approach (Items registering their description state with the List) would | ||
| // work but adds registration/unregistration callbacks, a new provider, and re-renders when | ||
| // the count changes. Not worth the complexity for a derived boolean. | ||
| // | ||
| // Two querySelector calls after render is trivially cheap compared to what the browser | ||
| // was doing on every DOM mutation with `:has()`. | ||
| useIsomorphicLayoutEffect(() => { | ||
| const list = listRef.current | ||
| if (!list) return | ||
| const hasMixed = | ||
| list.querySelector('[data-has-description="true"]') !== null && | ||
| list.querySelector('[data-has-description="false"]') !== null | ||
| const current = list.getAttribute('data-mixed-descriptions') | ||
| if (hasMixed && current !== 'true') { | ||
| list.setAttribute('data-mixed-descriptions', 'true') | ||
| } else if (!hasMixed && current !== null) { | ||
| list.removeAttribute('data-mixed-descriptions') | ||
| } | ||
| }) | ||
hectahertz marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| return ( | ||
| <ListContext.Provider value={listContextValue}> | ||
| {slots.heading} | ||
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.