perf(Button): replace :has(.Visual) with data-no-visuals attribute#7597
perf(Button): replace :has(.Visual) with data-no-visuals attribute#7597hectahertz wants to merge 3 commits intomainfrom
Conversation
hectahertz
requested review from
Copilot and
francinelucca
and removed request for
Copilot
🦋 Changeset detectedLatest commit: 8d0ae69 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 |
github-actions
bot
added
the
integration-tests: recommended
|
github-actions
bot
requested a deployment
to
storybook-preview-7597
There was a problem hiding this comment.
Pull request overview
This PR improves performance in @primer/react’s Button link variant styling by replacing expensive :has(.Visual) selectors with constant-time [data-no-visuals] / :not([data-no-visuals]) attribute selectors, leveraging an attribute already set by ButtonBase.
Changes:
- Replace 4 occurrences of
:has(.Visual)/:not(:has(.Visual))in link-underline styling with[data-no-visuals]selectors. - Add a patch changeset documenting the perf improvement.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/react/src/Button/ButtonBase.module.css | Swaps :has(.Visual)-based underline logic for [data-no-visuals] attribute selectors in link variant blocks. |
| .changeset/perf-button-css-has-selector.md | Patch changeset entry for the Button perf improvement. |
Comments suppressed due to low confidence (1)
packages/react/src/Button/ButtonBase.module.css:606
- Same
data-no-visualsvs:has(.Visual)behavior change applies here: in a loading state with no provided visuals/actions, the loading spinner adds a.Visualdescendant butdata-no-visualsremains present, so this block will apply where the old:not(:has(.Visual))block would not. If the intent is to keep loading behavior consistent with the pre-change selectors, consider excluding[data-loading='true']from the[data-no-visuals]branch (or adjusting how the attribute is computed).
&[data-no-visuals] {
text-decoration: none;
background-image: none;
}
&:not([data-no-visuals]) {
francinelucca
left a comment
francinelucca
left a comment
There was a problem hiding this comment.
LGTM, curious about copilot's review comment though 👀
|
@francinelucca I looked into it and it's safe. During loading the button content gets replaced by a spinner and |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14854 |
Closes #
Replaces 4 CSS
:has(.Visual)/:not(:has(.Visual))selectors in the Button link variant with[data-no-visuals]/:not([data-no-visuals])attribute selectors.ButtonBase.tsxalready setsdata-no-visualson the button element when no visuals are present, but the link variant underline styling in CSS was still using:has(.Visual)to detect them.:has()forces the browser to re-evaluate style on the parent whenever any descendant's class list changes, while attribute selectors only check the element itself.This is a continuation of #7327 which went stale. Same approach, rebased on current
main.Related PRs: #7556 (merged, ActionList
:has()fix), #7327 (stale, previous attempt at this exact fix)Changelog
New
N/A
Changed
[data-no-visuals]attribute selectors instead of:has(.Visual)for visual detectionRemoved
N/A
Rollout strategy
Testing & Reviewing
Pure CSS selector replacement. The
data-no-visualsattribute was already being set inButtonBase.tsx, so this is a CSS-only change. Specificity is preserved: both:has(.Visual)and[data-no-visuals]have specificity 0,1,0.data-a11y-link-underlinesattribute on an ancestor to verify both branches workMerge checklist