perf(Banner): replace :has(.BannerActions) with data-has-actions attribute#7598
perf(Banner): replace :has(.BannerActions) with data-has-actions attribute#7598hectahertz wants to merge 2 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: d5cf5b0 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 from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14851 |
4846a67 to
d5cf5b0
Compare
There was a problem hiding this comment.
Pull request overview
This PR improves Banner styling performance by eliminating :has(.BannerActions) selectors (which can trigger expensive descendant checks during style recalculation) and replacing them with a parent-scoped [data-has-actions] attribute set by Banner.tsx.
Changes:
- Add
data-has-actionsto the Banner root element whenprimaryActionorsecondaryActionis provided. - Replace the Banner CSS
:has()selectors with[data-has-actions]attribute selectors. - Add a patch changeset documenting the performance-oriented selector change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/Banner/Banner.tsx | Exposes action presence via data-has-actions on the <section> root to avoid CSS descendant scanning. |
| packages/react/src/Banner/Banner.module.css | Swaps :has(.BannerActions) / :has(.BannerActionsContainer) selectors for [data-has-actions] equivalents. |
| .changeset/perf-banner-css-has-selector.md | Adds a patch changeset describing the Banner CSS selector performance change. |
| aria-label={labelledBy ? undefined : label} | ||
| className={clsx(className, classes.Banner)} | ||
| data-dismissible={onDismiss ? '' : undefined} | ||
| data-has-actions={hasActions ? '' : undefined} | ||
| data-title-hidden={hideTitle ? '' : undefined} |
There was a problem hiding this comment.
data-has-actions is a new rendered attribute that now drives CSS behavior, but there’s no unit test asserting it’s present only when primaryAction/secondaryAction are provided (and absent otherwise). Adding a couple of assertions in Banner.test.tsx would help prevent accidental regressions (e.g., refactors to the hasActions logic).
Closes #
Replaces 4 CSS
:has(.BannerActions)selectors in Banner with[data-has-actions]attribute selectors.Banner.tsxalready computeshasActions = primaryAction || secondaryActionin render but wasn't exposing it as a data attribute. This PR addsdata-has-actionsto the<section>root and swaps out the:has()selectors for attribute checks.Same pattern as #7556 (ActionList) and #7597 (Button).
Selectors replaced:
.Banner[data-title-hidden]:not(:has(.BannerActions))(2 instances, content/icon sizing):where(.Banner):has(.BannerActions)(dismiss margin).BannerContainer:has(.BannerActionsContainer)(container query grid rows)Changelog
New
N/A
Changed
[data-has-actions]attribute selectors instead of:has(.BannerActions)for action detectionRemoved
N/A
Rollout strategy
Testing & Reviewing
One new
data-has-actionsattribute on the Banner root, rest is CSS selector replacement. Specificity is preserved.main@container banner (max-width: 500px)breakpoint with actions presentMerge checklist