Replace dynamic overflow with horizontal scroll in UnderlineNav#7648
Open
iansan5653 wants to merge 74 commits intomainfrom
Open
Replace dynamic overflow with horizontal scroll in UnderlineNav#7648iansan5653 wants to merge 74 commits intomainfrom
UnderlineNav#7648iansan5653 wants to merge 74 commits intomainfrom
Conversation
…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>
…ate to detect overflow
…er themselves for the menu
…-nav-full-css-spike
…r` at item level
…mer/react into underline-nav-full-css-spike
🦋 Changeset detectedLatest commit: 2a4484d 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
Contributor
|
13 tasks
Contributor
There was a problem hiding this comment.
Pull request overview
This PR simplifies UnderlineNav overflow behavior by removing the dynamic “More” overflow menu and switching to a horizontally scrollable container (with icon-hiding when space is tight), aligning with the goal of reducing layout shifts and complexity.
Changes:
- Replaced JS-driven overflow + “More” menu logic in
UnderlineNavwith a horizontal scroll container and overflow-driven icon hiding. - Updated shared underline tabbed interface styling/markup to support the new layout and icon-hiding strategy.
- Updated stories/tests/e2e snapshots in-progress to reflect the new overflow model.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/internal/components/UnderlineTabbedInterface.tsx | Removes iconsVisible prop usage and allows UnderlineItemList className merging. |
| packages/react/src/internal/components/UnderlineTabbedInterface.module.css | Updates layout/spacing and moves icon-hiding to a data-hide-icons attribute on the wrapper. |
| packages/react/src/experimental/UnderlinePanels/UnderlinePanels.tsx | Switches from data-icons-visible to data-hide-icons for icon visibility control. |
| packages/react/src/experimental/UnderlinePanels/UnderlinePanels.module.css | Removes now-redundant icon-hiding rule (handled by shared styles). |
| packages/react/src/UnderlineNav/styles.ts | Deletes overflow-menu-related inline style exports (no longer needed). |
| packages/react/src/UnderlineNav/UnderlineNavItem.tsx | Adds “scroll selected item into view” behavior; removes old measurement logic. |
| packages/react/src/UnderlineNav/UnderlineNavContext.tsx | Removes width/icon visibility state from context; keeps loadingCounters. |
| packages/react/src/UnderlineNav/UnderlineNav.tsx | Replaces overflow menu implementation with scroll container + ResizeObserver-based icon hiding. |
| packages/react/src/UnderlineNav/UnderlineNav.test.tsx | Adjusts tests for new structure and viewport-sensitive behavior. |
| packages/react/src/UnderlineNav/UnderlineNav.module.css | Implements scroll container styling + progressive icon-hiding logic. |
| packages/react/src/UnderlineNav/UnderlineNav.interactions.stories.tsx | Partially updated selectors; still structured around the removed “More” menu. |
| packages/react/src/UnderlineNav/UnderlineNav.features.stories.tsx | Updates overflow story setup to reflect scroll-based overflow behavior. |
| e2e/components/UnderlineNav.test.ts | Re-enables screenshots but still asserts “More” menu interactions (now removed). |
| .changeset/underline-nav-css-overflow.md | Adds a minor changeset documenting the behavior change. |
Comment on lines
+63
to
+66
| // Putting the resizeobserver on the list ensures it runs if the contents change; | ||
| // the container is fullwidth but the list is elastic | ||
| const list = scrollContainer.current.querySelector('ul') | ||
| if (!list) return |
Comment on lines
+113
to
114
| const moreBtn = canvas.getByRole('button', {name: 'More items'}) | ||
| userEvent.hover(moreBtn) |
Comment on lines
204
to
+214
| await page.setViewportSize({width: viewports['primer.breakpoint.sm'], height: 768}) | ||
| await page.locator('button', {hasText: 'More Repository Items'}).waitFor() | ||
| await page.locator('button', {hasText: 'More items'}).waitFor() | ||
|
|
||
| // Resize | ||
| // expect(await page.screenshot()).toMatchSnapshot() | ||
| expect(await page.screenshot()).toMatchSnapshot() | ||
|
|
||
| await page.getByRole('button', {name: 'More Repository Items'}).click() | ||
| // expect(await page.screenshot()).toMatchSnapshot() | ||
| await page.getByRole('button', {name: 'More items'}).click() | ||
| expect(await page.screenshot()).toMatchSnapshot() | ||
|
|
||
| await page.getByRole('link', {name: 'Settings (10)'}).click() | ||
| // expect(await page.screenshot()).toMatchSnapshot() | ||
| await page.getByRole('menuitem', {name: 'Settings (10)'}).click() | ||
| expect(await page.screenshot()).toMatchSnapshot() |
Comment on lines
+62
to
+64
| /** Registry of currently-overflowing underline items. If an item is not overflowing, its value will be `null`. */ | ||
| export const UnderlineNavItemsRegistry = createDescendantRegistry<UnderlineNavItemProps | null>() | ||
|
|
Comment on lines
+66
to
+82
| // Walk up the offset parent chain to get the true left offset relative to `container` | ||
| let offsetLeft = 0 | ||
| let el: HTMLElement | null = descendant | ||
| while (el && el !== container) { | ||
| offsetLeft += el.offsetLeft | ||
| el = el.offsetParent as HTMLElement | null | ||
| } | ||
|
|
||
| // scrollIntoView would be more convenient but would scroll the entire page unless we pass `options.container`, | ||
| // for which browser support is very limited | ||
| const descendantLeft = offsetLeft - container.scrollLeft | ||
| const descendantRight = descendantLeft + descendant.offsetWidth | ||
|
|
||
| const containerWidth = container.clientWidth | ||
|
|
||
| if (descendantLeft < 0) container.scrollLeft += descendantLeft | ||
| else if (descendantRight > containerWidth) container.scrollLeft += descendantRight - containerWidth |
Comment on lines
+9
to
+13
| /* Progressive enhancement: Detect overflow using scroll-based animations. | ||
| This lets us calculate the icon visibility during SSR. */ | ||
| animation: detect-overflow linear; | ||
| animation-timeline: scroll(self inline); | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Alternative approach to #7506; just get rid of the 'More' menu and scroll instead.
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist