Skip to content

perf(Button): replace :has() icon-only-counter selector with data attribute - #7893

Merged
jonrohan merged 2 commits into
mainfrom
perf/button-icon-only-counter
May 28, 2026
Merged

perf(Button): replace :has() icon-only-counter selector with data attribute#7893
jonrohan merged 2 commits into
mainfrom
perf/button-icon-only-counter

Conversation

@mattcosta7

Copy link
Copy Markdown
Contributor

Closes #

Replaces the icon-only-with-counter :has(...):not(:has(...)) selector on ButtonBase with a JS-derived data-icon-only-counter attribute computed from props at render time. Same visual output; reduces the per-Button style-recalculation cost.

Before:

&:where([data-has-count]):has([data-component='leadingVisual']):not(:has([data-component='text'])) {
padding-inline:var(--control-medium-paddingInline-condensed);
}

After:

data-icon-only-counter={count!==undefined&&LeadingVisual&&!children ? true : undefined}
&:where([data-icon-only-counter]) {
padding-inline:var(--control-medium-paddingInline-condensed);
}

:has() + :not(:has()) compound selectors are among the most expensive selectors in the CSS engine, and they evaluate against every .ButtonBase instance on every DOM mutation inside the button. Buttons render dozens to hundreds of times per page in real GitHub views, so this is a measurable cost. The equivalent JS check is a constant-time prop comparison.

Changelog

New

  • data-icon-only-counter attribute on ButtonBase when an icon-only Button has a count

Changed

  • Internal: ButtonBase no longer relies on :has() for icon-only-with-counter styling

Removed

  • The :has(...):not(:has(...)) compound selector from ButtonBase.module.css

Rollout strategy

  • Patch release
  • Minor release
  • Major release; if selected, include a written rollout or migration plan
  • None; if selected, include a brief description as to why

Testing & Reviewing

No visual changes expected. The new attribute is only set when the existing :has() selector would have matched (count set + leadingVisual rendered + no children).

Existing Button unit tests pass locally (138 tests across Button + ActionList suites).

Part of a small series replacing :has() selectors on high-frequency components with JS-derived data attributes. A follow-up PR will apply the same pattern to ActionList.

Merge checklist

…ribute
Computes 'icon-only with counter' state from props in ButtonBase and emits a data-icon-only-counter attribute, replacing the :where([data-has-count]):has([data-component='leadingVisual']):not(:has([data-component='text'])) compound selector. Same visual output; reduces style-recalculation cost on pages that render many Buttons.
CopilotAI review requested due to automatic review settings May 28, 2026 15:17
@mattcosta7
mattcosta7 requested a review from a team as a code ownerMay 28, 2026 15:17
@changeset-bot

changeset-botBot commented May 28, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 629c7d2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
NameType
@primer/reactPatch

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

@github-actionsgithub-actionsBot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label May 28, 2026
@github-actions

Copy link
Copy Markdown
Contributor

⚠️ Action required

👋 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. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR optimizes @primer/react’s ButtonBase icon-only-with-counter styling by replacing an expensive :has(...):not(:has(...)) CSS selector with a render-time data-icon-only-counter attribute, reducing style recalculation work on pages with many Buttons.

Changes:

  • Add data-icon-only-counter to ButtonBase when count + leadingVisual are present and there is no text content.
  • Update ButtonBase.module.css to target [data-icon-only-counter] instead of the :has()/:not(:has()) compound selector.
  • Add a patch changeset describing the internal perf improvement and the new attribute.
Show a summary per file
FileDescription
packages/react/src/Button/ButtonBase.tsxComputes and sets data-icon-only-counter from props at render time.
packages/react/src/Button/ButtonBase.module.cssReplaces the expensive :has() selector with an attribute selector for icon-only + counter padding.
.changeset/perf-button-icon-only-counter.mdDeclares a patch release and documents the change/new attribute.

Copilot's findings

  • Files reviewed: 3/3 changed files
  • Comments generated: 1

Comment threadpackages/react/src/Button/ButtonBase.tsx
@mattcosta7mattcosta7 self-assigned this May 28, 2026
Asserts the data-icon-only-counter attribute is set only when count + leadingVisual are present and there are no children. Locks in the JS contract the icon-only-counter styling now depends on.
@github-actions
github-actionsBottemporarily deployed to storybook-preview-7893 May 28, 2026 15:27 Inactive
@github-actions
github-actionsBottemporarily deployed to storybook-preview-7893 May 28, 2026 15:37 Inactive
@mattcosta7
mattcosta7 requested a review from jonrohanMay 28, 2026 16:51
@jonrohanjonrohan added the Canary Release Apply this label when you want CI to create a canary release of the current PR label May 28, 2026
@jonrohan
jonrohan enabled auto-merge May 28, 2026 18:57
@jonrohanjonrohan added the integration-tests: skipped manually Changes in this PR do not require an integration test label May 28, 2026
@jonrohan

Copy link
Copy Markdown
Member

Adding an attribute doesn't really need an integration test

@jonrohan
jonrohan added this pull request to the merge queueMay 28, 2026
Merged via the queue into main with commit 0eef204May 28, 2026
67 checks passed
@jonrohan
jonrohan deleted the perf/button-icon-only-counter branch May 28, 2026 19:12
@primerprimerBot mentioned this pull request May 28, 2026
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Canary ReleaseApply this label when you want CI to create a canary release of the current PRintegration-tests: recommendedThis change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpmintegration-tests: skipped manuallyChanges in this PR do not require an integration test

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@mattcosta7@jonrohan