Skip to content

Conversation

pksjce
Copy link
Contributor

@pksjce pksjce commented Aug 25, 2025

Closes #https://github.com/github/primer/issues/5122

Changelog

Breadcrumbs now show an overflow menu with the following features

  • If no of items in the breadcrumbs is >5 then we show the menu
  • If the screen width can't accomodate the items then we show the menu
  • if overflow==="menu-with-root" show the root item.

https://www.loom.com/share/8902798416b741c9959fc7bb501185a5

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

@Copilot Copilot AI review requested due to automatic review settings August 25, 2025 11:48
@pksjce pksjce requested a review from a team as a code owner August 25, 2025 11:48
@pksjce pksjce requested a review from francinelucca August 25, 2025 11:48
Copy link

changeset-bot bot commented Aug 25, 2025

🦋 Changeset detected

Latest commit: 2854259

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
@primer/react Patch
@primer/styled-react Patch

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 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 Aug 25, 2025
Copy link
Contributor

👋 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!

Copilot

This comment was marked as spam.

Copy link
Contributor

github-actions bot commented Aug 25, 2025

size-limit report 📦

Path Size
packages/react/dist/browser.esm.js 90.72 KB (+1% 🔺)
packages/react/dist/browser.umd.js 91.03 KB (+1.14% 🔺)

@github-actions github-actions bot requested a deployment to storybook-preview-6664 September 1, 2025 03:05 Abandoned
@github-actions github-actions bot requested a deployment to storybook-preview-6664 September 1, 2025 06:14 Abandoned
@github-actions github-actions bot temporarily deployed to storybook-preview-6664 September 1, 2025 06:24 Inactive
@pksjce
Copy link
Contributor Author

pksjce commented Sep 1, 2025

Summary of changes since last review

  • @strackoverflow - Nice catch on the looping behavior. This is now resolved.

  • @francinelucca

    • I have added the ally recommendations.
    • Also added the feature flag - primer_react_breadcrumbs_overflow_menu
    • Removed the extra changeset file
  • @joshblack

    • I have fixed the issue of wrong update on saving the file
    • Worked on the memoized resize calculation
    • Used Details to implement disclosure pattern
    • Changed the overflow property to have values menu and menu-with-root eliminating hideRoot boolean. cc- Thanks @siddharthkp
    • Removed unnecessary slice

    Notes on Accessibility improvements

  • ActionMenu has been replaced by an implementation using Details and Details.Summary.

  • aria-expanded property moves from false to true as required.

  • There are no more menuitem roles. All are li tags with the inbuilt role

  • The tooltip has been positioned on the right.

  • Esc and outside click will work to close the menu

  • Tabbing is continous inside and outside the menu without any focus traps.

Advice needed

  • Tabbing into the menubutton will not open it automatically nor Tabbing out will close it automatically. This was recommended in the session but did not seem standard to disclosure implementations eg UnderlineNav.

Copy link
Contributor

@langermank langermank left a comment

Choose a reason for hiding this comment

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

This is purely a CSS review! Will take a closer look at behavior next.

}

.MenuOverlay {
position: absolute;
Copy link
Contributor

Choose a reason for hiding this comment

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

We need to have an offset between the trigger and the menu, but I'm not sure how you want to implement that with this custom overlay. The variable for this is var(--overlay-offset) which is 4px, so you might need to use a calc to achieve that.

<BoxWithFallback as="nav" className={clsx(className, classes.BreadcrumbsBase)} aria-label="Breadcrumbs" sx={sxProp}>
<BreadcrumbsList>{wrappedChildren}</BreadcrumbsList>
</BoxWithFallback>
)
}

const ItemSeparator = () => {
return (
<span className={classes.ItemSeparator}>
Copy link
Contributor

Choose a reason for hiding this comment

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

You might need some labeling on this for a11y support (just a hunch, would look it up)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not focusable. I'm not sure if the label is required. What would you expect in the label?
cc - @janmaarten-a11y

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants