-
Notifications
You must be signed in to change notification settings - Fork 52
Fix separator rendering bug in SubNav #1120
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
Conversation
🦋 Changeset detectedLatest commit: a66f2c4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 packages
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 |
🟢 No design token changes found |
🟢 No visual differences foundOur visual comparison tests did not find any differences in the UI. |
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
Fixes a layout shift bug in SubNav component where separators were conditionally rendered based on client-side state, causing hydration issues in SSR environments. The fix replaces JavaScript-based conditional rendering with CSS-based visibility control to ensure consistent DOM structure during initial render and hydration.
- Replaced
isLarge
condition logic with CSS equivalents for separator visibility - Improved optical alignment of separator elements
- Added comprehensive visual regression tests for no active links scenario
Reviewed Changes
Copilot reviewed 6 out of 19 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
packages/react/src/SubNav/SubNav.tsx | Replaced conditional rendering with CSS classes and improved separator structure |
packages/react/src/SubNav/SubNav.module.css | Added CSS rules for separator visibility and optical alignment |
packages/react/src/SubNav/SubNav.test.tsx | Removed test case that's no longer applicable with CSS-based approach |
packages/react/src/SubNav/SubNav.features.stories.tsx | Added new story variants for testing no active links scenario |
packages/react/src/SubNav/SubNav.visual.spec.ts | Added comprehensive visual regression tests for multiple locales |
.changeset/poor-balloons-jam.md | Added changeset documenting the layout shift fix |
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.
LGTM
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.
Works great!
Summary
Resolves https://github.com/github/primer/issues/5548
Fixes a layout shift problem that occurs in certain SSR environments where an observable delay in hydration led to the separator in SubNav loading in client side and after the adjacent elements were initially rendered on page.
List of notable changes:
isLarge
condition inSubNav
for rendering the separator and certainchildren
. This was causing the initial render to exclude an important DOM node in dotcom, until after hydration was complete.isLarge
logic with CSS equivalents. Does the same thing.What should reviewers focus on?
Steps to test:
Contributor checklist:
update snapshots
label to the PR)Reviewer checklist:
Screenshots:
Demo of the fix running in dotcom 👇
Screen.Recording.2025-07-28.at.16.38.36.mov
Screen.Recording.2025-07-28.at.16.38.09.mov
Optical alignment: