-
Notifications
You must be signed in to change notification settings - Fork 652
UnderlineNav: Add overflow: hidden when calculating items for overflow menu
#7504
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
👋 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 |
🦋 Changeset detectedLatest commit: d9aab4b 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.
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 aims to reduce layout shift (CLS) in UnderlineNav by hiding overflow while the component calculates which items should move into the overflow (“More”) menu (fix for primer issue #6291).
Changes:
- Add a
readyprop to the sharedUnderlineWrapperand emit adata-readyattribute. - Update
UnderlineWrapperCSS to hide overflow untildata-ready='true'. - Track
isReadyinUnderlineNavand pass it through toUnderlineWrapper. - Add a patch changeset entry.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/react/src/internal/components/UnderlineTabbedInterface.tsx | Adds ready prop and data-ready attribute on the shared wrapper component. |
| packages/react/src/internal/components/UnderlineTabbedInterface.module.css | Hides overflow by default and restores it when data-ready='true'. |
| packages/react/src/UnderlineNav/UnderlineNav.tsx | Introduces isReady state and passes it to UnderlineWrapper. |
| .changeset/clever-geese-cover.md | Patch changeset documenting the CLS-related UnderlineNav update. |
Comments suppressed due to low confidence (1)
packages/react/src/UnderlineNav/UnderlineNav.tsx:170
- The new
isReady/readygating is user-visible behavior meant to prevent CLS, but it isn’t covered by a unit test. Since this component already has a comprehensive test suite, it would be good to add a test asserting the wrapper is initially non-ready (overflow hidden) and becomes ready after the first successful overflow calculation/resize observer pass.
// Track whether the initial overflow calculation is complete to prevent CLS
const [isReady, setIsReady] = useState(false)
packages/react/src/internal/components/UnderlineTabbedInterface.module.css
Outdated
Show resolved
Hide resolved
packages/react/src/internal/components/UnderlineTabbedInterface.tsx
Outdated
Show resolved
Hide resolved
…e.module.css Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…e.tsx Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
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/13031 |
| box-shadow: inset 0 -1px var(--borderColor-muted); | ||
|
|
||
| /* Hide overflow until calculation is complete to prevent CLS */ | ||
| overflow: visible; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can do this entirely within css and avoid any of the useEffect() updates, just with something like:
.UnderlineWrapper {
&:not(:has([class*='prc-UnderlineNav-MoreButton-'])) {
overflow-x: hidden;
}
}You might need to update the classNames a bit (or move it to the UnderlineNav component directly)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice! Although probably the same thing perf wise because the MoreButton appears after a javascript pass as well, right?
Thinking out loud, do we need to test this for cases where there is an overflow because of icons and counters, so the MoreButton may never appear, or is that already covered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud, do we need to test this for cases where there is an overflow because of icons and counters, so the MoreButton may never appear,
Can the component still overflow without the menu? I haven't seen that, as I assumed that we also took those into account when determining whether to show the menu or not.
iansan5653
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running this locally, I noticed that data-ready is not set to true, even though it is in the Storybook preview.
I tracked it down to this condition:
if (!isReady && widths.length > 0 && widths.length === validChildren.length) {
setIsReady(true)
}
Logging widths.length vs validChildren.length shows that widths is twice as long as validChildren. The fact that it's exactly double smells like a potential StrictMode bug.
| <Component | ||
| className={clsx(classes.UnderlineWrapper, className)} | ||
| ref={ref as ForwardedRef<HTMLDivElement>} | ||
| data-ready={ready === undefined ? undefined : ready ? 'true' : 'false'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More descriptive name please. data-hydrated / data-overflow-handled?
siddharthkp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments, approving in advance
Co-authored-by: Siddharth Kshetrapal <siddharthkp@github.com>
liuliu-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Closes https://github.com/github/primer/issues/6291
Credits to @siddharthkp & @francinelucca for the original PR: #7463 ✨
Changelog
New
Changed
overflow: hiddento container when loading itemsRollout strategy
Testing & Reviewing
Merge checklist