feat(ActionList): update loading styles for specific child elements #7468
Conversation
🦋 Changeset detectedLatest commit: 79b2187 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Or, apply the |
github-actions
bot
added
the
integration-tests: recommended
| } | ||
|
|
||
| &:where([data-loading='true']), | ||
| &:has([data-loading='true']) { |
There was a problem hiding this comment.
is the [data-loading] really at any level of depth, or is it possibly on the ListItem wrappers directly? that would probably be another nice win
There was a problem hiding this comment.
Theres is a .TrailingAction data-loading that can happen, but we can make the has more specific here to avoid scanning all the descendants.
github-actions
bot
requested a deployment
to
storybook-preview-7468
Optimizes CSS selector performance in ActionList by replacing expensive universal selectors (`*`) and broad `:has()` queries with targeted class selectors.
There was a problem hiding this comment.
Pull request overview
Improves ActionList loading-state CSS performance by replacing expensive universal/:has() selectors with more targeted class-based selectors.
Changes:
- Replaced
& *loading-state styling with a curated set of ActionList child classes. - Removed the broad
:has([data-loading='true'])loading selector path. - Added a changeset documenting the performance optimization.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/ActionList/ActionList.module.css | Updates loading-state selectors to reduce matching work and avoid broad descendant scans. |
| .changeset/orange-walls-buy.md | Publishes the change as a patch with a note about selector performance optimization. |
| &:where([data-loading='true']), | ||
| &:has([data-loading='true']) { | ||
| & * { | ||
| & > [data-loading='true'] { |
There was a problem hiding this comment.
& > [data-loading='true'] likely never matches with the current ActionList.Item markup. data-loading is set on the <li> itself (Item.tsx:210), and when TrailingAction is loading the attribute is on the nested button inside .TrailingAction, not on a direct child of the <li>. Consider removing this selector (keeping only &:where([data-loading='true'])) to avoid dead/incorrect CSS, or update it to match an actual DOM structure that needs support.
See below for a potential fix:
&:where([data-loading='true']) {
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/11637 |
Part of https://github.com/github/pull-requests/issues/22213
Summary
Optimizes CSS selector performance in ActionList by replacing expensive universal selectors (
*) and broad:has()queries with targeted class selectors.:is(.ActionListItem:not(:has([aria-disabled], [disabled]), [data-has-subitem="true"]):where([data-loading="true"]), .ActionListItem:not(:has([aria-disabled], [disabled]), [data-has-subitem="true"]):has([data-loading="true"])) *Problem
During LCP (Largest Contentful Paint) analysis, we identified a selector in ActionList that was attempting to match ~200k elements but matching none:
The issues were:
& *) - Forces the browser to check every descendant element:has([data-loading='true'])- Scans all descendants looking for thedata-loadingattributeSolution
Replaced
& *with explicit class targets:.ItemLabel.Description.LeadingVisual.TrailingVisual.VisualWrapRemoved
:has([data-loading='true'])entirely - After investigation,data-loadingis set directly on the<li>element viaItem.tsx, so the:has()variant was unnecessary. The separate rule at line 369 already handles TrailingAction-specific loading states.Related Issue(s)
Before/After
&:where([data-loading='true']), &:has([data-loading='true']) { & * { ... } }&:where([data-loading='true']), & [data-loading='true'] { & .ItemLabel, & .Description, ... { ... } }:has()scopedata-loadingattributeTesting & Verification
npm run build)npm test- 505 tests)npx stylelint)loadingpropMerge checklist
Screenshots
N/A - This is a performance optimization with no visual changes.