Uh oh!
There was an error while loading. Please reload this page.
perf(Dialog): replace :has(.Footer) with data-has-footer attribute - #7599
Conversation
🦋 Changeset detectedLatest commit: af1f67f 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 |
|
There was a problem hiding this comment.
Pull request overview
This PR improves Dialog CSS performance by avoiding a parent :has() selector for footer detection and instead exposing footer presence via a data-has-footer attribute on the Dialog root.
Changes:
- Add
data-has-footerto the Dialog root element when a footer is rendered. - Replace
.Dialog:has(.Footer)with.Dialog[data-has-footer]in Dialog CSS for the scroll border rule. - Add a changeset documenting the patch change.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/react/src/Dialog/Dialog.tsx | Adds data-has-footer attribute derived from the computed footer node. |
| packages/react/src/Dialog/Dialog.module.css | Updates footer border detection selector to use [data-has-footer] instead of :has(.Footer). |
| .changeset/perf-dialog-css-has-selector.md | Adds a patch changeset entry describing the selector/attribute change. |
| {...positionDataAttributes} | ||
| data-width={width} | ||
| data-height={height} | ||
| data-has-footer={footer ? '' : undefined} |
There was a problem hiding this comment.
data-has-footer is derived from footer ? ... which uses a truthiness check. Since footer is a ReactNode, valid values like 0 or '' would incorrectly omit the attribute. Prefer a nullish check (e.g. set the attribute when footer != null) to match “footer rendered” semantics more reliably.
| data-has-footer={footer ? '' : undefined} | |
| data-has-footer={footer !=null? '' : undefined} |
| {...positionDataAttributes} | ||
| data-width={width} | ||
| data-height={height} | ||
| data-has-footer={footer ? '' : undefined} |
There was a problem hiding this comment.
A new DOM contract is introduced (data-has-footer) and drives styling, but there are no unit tests asserting the attribute is present when a footer is rendered and absent when renderFooter/footer slot returns null. Adding a couple of assertions in the existing Dialog test suite would help prevent regressions.
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/14866 |
francinelucca
left a comment
There was a problem hiding this comment.
mostly LGTM, just was wondering about the TextInput file
Uh oh!
There was an error while loading. Please reload this page.
hectahertz
commented
Mar 4, 2026
Good catch! Removed those, they already shipped in #7600. |
437bd14 to
fb8e57aCompare0e1f19a to
fb8e57aCompareUh oh!
There was an error while loading. Please reload this page.
Closes #
Replaces
.Dialog:has(.Footer)with.Dialog[data-has-footer]for the scrollable body border detection rule.Dialog already resolves whether a footer exists in JS via
useSlots+renderFooter, but wasn't exposing it as a data attribute. This PR addsdata-has-footeron the dialog div and swaps the:has()selector for an attribute check.The more expensive scroll-lock
:has()selector (body:has(.Dialog.DisableScroll)) is already guarded behind a feature flag (primer_react_css_has_selector_perf) and gated with:not([data-dialog-scroll-optimized]), so it's not addressed here.Other open perf PRs:#7598, #7597, #7554, #7550, #7549, #7545, #7544
Changelog
New
N/A
Changed
[data-has-footer]attribute selector instead of:has(.Footer)for footer border detectionRemoved
N/A
Rollout strategy
Testing & Reviewing
One new
data-has-footerattribute on the dialog div, one CSS selector change. Theanimation-timeline: scroll(self)border logic is unchanged.Merge checklist