fix(Breadcrumbs): Do not show overflow menu when breadcrumb has only two items#7797
Conversation
🦋 Changeset detectedLatest commit: a2680e6 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 |
|
|
could check the change in the failed vrt on dotcom: |
There was a problem hiding this comment.
Pull request overview
Adjusts Breadcrumbs overflow-menu behavior so that a narrow viewport does not force-collapse into an overflow menu when there are only two breadcrumb items, aligning the component with expected UX for small breadcrumb sets.
Changes:
- Update overflow calculation to only apply the narrow “min visible items = 1” rule when there are more than two items.
- Add Storybook feature stories covering the two-item
overflow="menu"andoverflow="menu-with-root"cases. - Add Playwright VRT coverage + new snapshot baselines for the two-item scenarios, and a patch changeset entry.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/Breadcrumbs/Breadcrumbs.tsx | Prevents narrow-mode overflow collapsing when there are only 2 items. |
| packages/react/src/Breadcrumbs/Breadcrumbs.features.stories.tsx | Adds stories to exercise two-item overflow props. |
| e2e/components/Breadcrumbs.test.ts | Adds VRT tests for two-item overflow scenarios. |
| .playwright/snapshots/components/Breadcrumbs.test.ts-snapshots/Breadcrumbs-TwoItemsOverflowMenu-narrow-linux.png | New VRT baseline for two-item overflow="menu" story. |
| .playwright/snapshots/components/Breadcrumbs.test.ts-snapshots/Breadcrumbs-TwoItemsOverflowMenuWithRoot-narrow-linux.png | New VRT baseline for two-item overflow="menu-with-root" story. |
| .changeset/free-wasps-drop.md | Patch changeset entry for the behavior fix. |
Copilot's findings
- Files reviewed: 4/6 changed files
- Comments generated: 2
| export const TwoItemsWithOverflowMenu = () => ( | ||
| <Breadcrumbs overflow="menu"> | ||
| <Breadcrumbs.Item href="#">Home</Breadcrumbs.Item> | ||
| <Breadcrumbs.Item href="#" selected> | ||
| Current Page | ||
| </Breadcrumbs.Item> | ||
| </Breadcrumbs> | ||
| ) | ||
|
|
||
| export const TwoItemsWithOverflowMenuWithRoot = () => ( | ||
| <Breadcrumbs overflow="menu-with-root"> | ||
| <Breadcrumbs.Item href="#">Home</Breadcrumbs.Item> | ||
| <Breadcrumbs.Item href="#" selected> | ||
| Current Page | ||
| </Breadcrumbs.Item> | ||
| </Breadcrumbs> | ||
| ) |
There was a problem hiding this comment.
The new Storybook exports are named as if an overflow menu is expected to render, but the PR’s stated behavior is that the overflow menu should not appear when there are only two items. Consider renaming these stories to reflect the intent (e.g., that overflow="menu"/"menu-with-root" does not render a menu with two items) to avoid confusing future readers and consumers of the story IDs.
| test.describe('Two Items With Overflow Menu', () => { | ||
| test('narrow viewport @vrt', async ({page}) => { | ||
| await visit(page, { | ||
| id: 'components-breadcrumbs-features--two-items-with-overflow-menu', | ||
| globals: { | ||
| colorScheme: 'light', | ||
| }, | ||
| }) | ||
|
|
||
| await page.setViewportSize({width: viewports['primer.breakpoint.xs'] - 1, height: 768}) | ||
| await expect(page).toHaveScreenshot('Breadcrumbs.TwoItemsOverflowMenu.narrow.png') | ||
| }) | ||
| }) | ||
|
|
||
| test.describe('Two Items With Overflow Menu With Root', () => { | ||
| test('narrow viewport @vrt', async ({page}) => { | ||
| await visit(page, { | ||
| id: 'components-breadcrumbs-features--two-items-with-overflow-menu-with-root', | ||
| globals: { | ||
| colorScheme: 'light', | ||
| }, | ||
| }) | ||
|
|
||
| await page.setViewportSize({width: viewports['primer.breakpoint.xs'] - 1, height: 768}) | ||
| await expect(page).toHaveScreenshot('Breadcrumbs.TwoItemsOverflowMenuWithRoot.narrow.png') | ||
| }) |
There was a problem hiding this comment.
Test/screenshot naming here suggests an overflow menu should be present ("Two Items With Overflow Menu" / "TwoItemsOverflowMenu"), but the intended behavior is that no overflow menu renders for two items. Renaming the describe block and screenshot baselines to match the expected UI would make the VRT coverage easier to understand and maintain.
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/19578 |
Closes #
Changelog
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist