Skip to content

Comments

Fix and refactor UnderlineNav to resolve CLS issues and improve performance/code quality#7506

Draft
iansan5653 wants to merge 38 commits intomainfrom
underline-nav-full-css-spike
Draft

Fix and refactor UnderlineNav to resolve CLS issues and improve performance/code quality#7506
iansan5653 wants to merge 38 commits intomainfrom
underline-nav-full-css-spike

Conversation

@iansan5653
Copy link
Contributor

@iansan5653 iansan5653 commented Feb 6, 2026

This refact/bugfix PR reworks UnderlineNav's internal logic to rely mostly on CSS for calculating overflow, eliminating layout shift issues. In addition, it removes the use of the React.Children API in favor of context-based solutions, and replaces the custom overflow menu with an ActionMenu.

See https://github.com/github/primer/issues/6291#issuecomment-3861950186 for context.

The CSS overflow logic works like this:

  1. Overflowing menu items wrap onto subsequent lines, which are clipped by their container using overflow: hidden, creating the illusion that they disappeared
  2. The initial visibility of the overflow menu button and tab icons is controlled by scroll-state container queries to detect overflow using pure CSS
  3. Pure CSS cannot, however, tell us what to render inside the overflow menu. For this, each list item registers itself using context:
    1. The parent component provides register/unregister functions through context
    2. Each item component registers an IntersectionObserver to update itself with the parent when it appears/disappears. They determine whether they are overflowed by checking ref.current.offsetHeight (which will be greater than zero for an item that has wrapped). If they are overflowed, they set aria-hidden/tabIndex="-1" and notify the parent via the registry.
    3. The parent renders menu items for all currently-overflowing items in the overflow menu
    4. If any item has ever overflowed, all tab icons are suppressed via React state for the rest of the lifecycle of the component (this prevents flickering caused by the tab icons creating/removing space)

The benefits are pretty significant:

  1. The browser will calculate overflow for us before we ever even paint on the page - no waiting on a JS bundle or React hydration (this benefit is shared with the lighter-weight approach)
  2. Calculations are nearly pure CSS, so performance is buttery smooth for the whole component lifecycle
  3. We get to clean up tons of complex code, remove several effects, and remove hacky Children APIs entirely
  4. Bonus: Component consumption is much less fragile. Without Children APIs, we no longer need direct children, so consumers can wrap their underline items in fragments and even wrapper components

However, there are some caveats / user-facing changes:

  1. The most significant is that we can no longer ensure that the current item will not overflow into the menu. This is impossible to implement with the new approach; even if we were to extract the current item and render it outside the overflow menu, there's no place to put it since it would just wrap out of view if rendered into the list. If rendered next to the list, it would be pushed too far to the right due to the space caused by wrapping items in the list. Instead, I've added some styling to indicate the current item inside the menu:
    screenshot of overflow menu item with bold text and orange border line
  2. The overflow menu button must be right-aligned, because the list of items will always take up as much space as possible before wrapping. Honestly I think this is a nice design improvement.
  3. Instead of moving the first item (last to overflow) into the overflow menu, we will truncate it with an ellipsis instead. This should almost never happen, as the first item would have to have a very long label.
  4. If the order of menu items changes after the initial render, the order of the items in the menu may not be correct. This should be a rare case (when would the order of items change?) but it is worth calling out. The registry-based approach depends on the order remaining the same.

Changelog

New

Changed

Removed

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

Merge checklist

TylerJDev and others added 8 commits February 6, 2026 15:05
…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>
@changeset-bot
Copy link

changeset-bot bot commented Feb 6, 2026

⚠️ No Changeset found

Latest commit: eef80ba

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@github-actions github-actions bot added the integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm label Feb 6, 2026
@github-actions
Copy link
Contributor

github-actions bot commented Feb 6, 2026

👋 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 integration-tests: skipped manually label to skip these checks.

@iansan5653 iansan5653 changed the base branch from main to tylerjdev/overflow-underlinenav February 6, 2026 20:28
@github-actions github-actions bot requested a deployment to storybook-preview-7506 February 6, 2026 20:31 Abandoned
@github-actions github-actions bot temporarily deployed to storybook-preview-7506 February 6, 2026 20:40 Inactive
@iansan5653
Copy link
Contributor Author

BTW, I think the flex inspector in Chrome's devtools does a really nice job of showing what's going on here / where the tabs are going:

Screen.Recording.2026-02-06.at.5.00.33.PM.mov

@iansan5653
Copy link
Contributor Author

@copilot Run and update VRT snapshots

Copy link
Contributor

Copilot AI commented Feb 20, 2026

@iansan5653 I've opened a new pull request, #7580, to work on those changes. Once the pull request is ready, I'll request review from you.

@iansan5653 iansan5653 added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label Feb 20, 2026
// expect(await page.screenshot()).toMatchSnapshot()

await page.setViewportSize({width: viewports['primer.breakpoint.sm'], height: 768})
await page.locator('button', {hasText: 'More Repository Items'}).waitFor()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the label of the overflow button to "More items", aligning with this issue for ActionBar: #7437

@iansan5653 iansan5653 added update snapshots 🤖 Command that updates VRT snapshots on the pull request and removed update snapshots 🤖 Command that updates VRT snapshots on the pull request labels Feb 20, 2026
@github-actions github-actions bot temporarily deployed to storybook-preview-7506 February 20, 2026 22:05 Inactive
@iansan5653 iansan5653 added update snapshots 🤖 Command that updates VRT snapshots on the pull request and removed update snapshots 🤖 Command that updates VRT snapshots on the pull request labels Feb 23, 2026
@primer
Copy link
Contributor

primer bot commented Feb 23, 2026

🤖 Lint issues have been automatically fixed and committed to this PR.

@github-actions github-actions bot temporarily deployed to storybook-preview-7506 February 23, 2026 17:15 Inactive
@primer
Copy link
Contributor

primer bot commented Feb 23, 2026

🤖 Lint and formatting issues have been automatically fixed and committed to this PR.

@github-actions github-actions bot temporarily deployed to storybook-preview-7506 February 23, 2026 17:26 Inactive
@primer
Copy link
Contributor

primer bot commented Feb 23, 2026

🤖 Lint issues have been automatically fixed and committed to this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

integration-tests: recommended This change needs to be tested for breaking changes. See https://arc.net/l/quote/tdmpakpm update snapshots 🤖 Command that updates VRT snapshots on the pull request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants