-
Notifications
You must be signed in to change notification settings - Fork 626
Breadcrumbs : Add overflow menu for responsive behavior #6561
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: a34f53f The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 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 |
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
size-limit report 📦
|
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 implements overflow handling for the Breadcrumbs component to manage cases where breadcrumb items exceed container width or count limits. The implementation adds a menu-based overflow system that intelligently hides intermediate breadcrumb items behind an ellipsis menu.
Key Changes
- Added
overflow
prop with 'wrap' (default) and 'menu' modes for different overflow behaviors - Implemented responsive overflow logic that adapts to container width using ResizeObserver
- Added
hideRoot
prop to control whether the first breadcrumb item should be preserved when overflowing
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
File | Description |
---|---|
packages/react/src/Breadcrumbs/Breadcrumbs.tsx | Core implementation adding overflow detection, menu creation, and responsive breadcrumb management |
packages/react/src/Breadcrumbs/Breadcrumbs.stories.tsx | New Storybook stories demonstrating different overflow scenarios and configurations |
packages/react/src/Breadcrumbs/Breadcrumbs.module.css | CSS updates to prevent wrapping when menu overflow is enabled |
.changeset/good-cougars-hug.md | Changeset file for tracking this feature addition |
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
if (listElement && listElement.children.length > 0 && itemWidths.length === 0) { | ||
const widths = Array.from(listElement.children).map(child => (child as HTMLElement).offsetWidth) | ||
setItemWidths(widths) | ||
} |
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.
The width calculation runs inside the overflow calculation effect, which could cause performance issues during frequent resizes. Consider memoizing the width calculation or moving it to a separate effect that only runs when the DOM structure changes.
} |
Copilot uses AI. Check for mistakes.
const widths = Array.from(listElement.children).map(child => (child as HTMLElement).offsetWidth) | ||
setItemWidths(widths) | ||
} | ||
const MENU_BUTTON_WIDTH = 50 // Approximate width of "..." button |
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.
Using a magic number for the menu button width makes the code fragile. Consider calculating this dynamically or defining it as a named constant at the module level with documentation explaining how this value was determined.
const MENU_BUTTON_WIDTH = 50 // Approximate width of "..." button | |
// Use module-level MENU_BUTTON_WIDTH constant (see top of file) |
Copilot uses AI. Check for mistakes.
// Target: 4 visible items + 1 menu = 5 total | ||
const itemsToHide = childArray.slice(0, childArray.length - 4) | ||
currentMenuItems = itemsToHide | ||
currentVisibleItems = childArray.slice(childArray.length - 4) |
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.
The hardcoded value of 5 items should be extracted as a configurable constant. This makes the component more flexible and the business logic clearer.
currentVisibleItems = childArray.slice(childArray.length - 4) | |
// If more than DEFAULT_MAX_VISIBLE_BREADCRUMBS items, start by reducing to max visible items (including menu) | |
if (childArray.length > DEFAULT_MAX_VISIBLE_BREADCRUMBS) { | |
// Target: (max - 1) visible items + 1 menu = max total | |
const itemsToHide = childArray.slice(0, childArray.length - (DEFAULT_MAX_VISIBLE_BREADCRUMBS - 1)) | |
currentMenuItems = itemsToHide | |
currentVisibleItems = childArray.slice(childArray.length - (DEFAULT_MAX_VISIBLE_BREADCRUMBS - 1)) |
Copilot uses AI. Check for mistakes.
setMenuItems(result.menuItems) | ||
setEffectiveHideRoot(result.effectiveHideRoot) | ||
} | ||
}, [childArray, overflow, containerWidth, hideRoot, itemWidths]) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
const handleResize = useCallback((entries: ResizeObserverEntry[]) => { | ||
if (entries[0]) { | ||
setContainerWidth(entries[0].contentRect.width) | ||
} |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: primer[bot] <119360173+primer[bot]@users.noreply.github.com> Co-authored-by: Josh Black <[email protected]>
Co-authored-by: Jon Rohan <[email protected]>
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: pksjce <[email protected]> Co-authored-by: Pavithra Kodmad <[email protected]> Co-authored-by: Jon Rohan <[email protected]>
…S modules (#6508) Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: hectahertz <[email protected]> Co-authored-by: Hector Garcia <[email protected]> Co-authored-by: Josh Black <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: primer[bot] <119360173+primer[bot]@users.noreply.github.com> Co-authored-by: Jon Rohan <[email protected]> Co-authored-by: Marie Lucca <[email protected]>
Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: hectahertz <[email protected]> Co-authored-by: Hector Garcia <[email protected]> Co-authored-by: Copilot <[email protected]> Co-authored-by: Josh Black <[email protected]>
Co-authored-by: Marie Lucca <[email protected]>
Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Marie Lucca <[email protected]> Co-authored-by: Marie Lucca <[email protected]> Co-authored-by: Josh Black <[email protected]>
I've messed up this PR beyond repair. I will cherrypick my commits and open a new one |
Closes #https://github.com/github/primer/issues/5122
Changelog
Breadcrumbs now show an overflow menu with the following features
hideRoot
is false then we show the root/overflow menu/leafhttps://www.loom.com/share/8902798416b741c9959fc7bb501185a5
New
Changed
Removed
Rollout strategy
Testing & Reviewing
Merge checklist