Replace ActionBar overflow calculations with CSS wrapping approach#7655
Replace ActionBar overflow calculations with CSS wrapping approach#7655iansan5653 wants to merge 30 commits intomainfrom
ActionBar overflow calculations with CSS wrapping approach#7655Conversation
🦋 Changeset detectedLatest commit: ad6ddff 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
|
There was a problem hiding this comment.
Pull request overview
This PR refactors ActionBar overflow handling away from JS width calculations and towards a CSS wrapping + overflow-clipping approach (similar to the UnderlineNav work), aiming to reduce flicker and improve SSR stability.
Changes:
- Replaces ResizeObserver/width math with CSS wrapping + scroll-driven overflow detection.
- Introduces a per-item overflow registration mechanism using
useSyncExternalStore+IntersectionObserver. - Adds a changeset for a minor release of
@primer/react.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| packages/react/src/ActionBar/ActionBar.tsx | Reworks overflow detection and overflow menu population to use descendant registry updates driven by wrapping/overflow state. |
| packages/react/src/ActionBar/ActionBar.module.css | Adds wrapping container + scroll-timeline based detection to control overflow button visibility. |
| .changeset/many-suns-promise.md | Declares the change as a minor release. |
Comments suppressed due to low confidence (1)
packages/react/src/ActionBar/ActionBar.tsx:433
refis cast toRefObjectfromforwardedRef, butforwardedRefcan be a callback ref at runtime.useActionBarItemreadsref.current, which will throw if a callback ref is passed. Consider using an internaluseRefplususeRefObjectAsForwardedRef/useMergedRefsso you always have a real object ref for measurements and can still forward refs safely.
export const ActionBarGroup = forwardRef(({children}: React.PropsWithChildren, forwardedRef) => {
const backupRef = useRef<HTMLDivElement>(null)
const ref = (forwardedRef ?? backupRef) as RefObject<HTMLDivElement>
const {id, isOverflowing} = useActionBarItem(
ref,
useMemo((): ChildProps => ({type: 'group'}), []),
| ) => { | ||
| const backupRef = useRef<HTMLButtonElement>(null) | ||
| const ref = (forwardedRef ?? backupRef) as RefObject<HTMLButtonElement> | ||
| const {isVisibleChild} = React.useContext(ActionBarContext) | ||
|
|
There was a problem hiding this comment.
ref is cast to RefObject from forwardedRef, but forwardedRef can be a callback ref at runtime. Since useActionBarItem and ActionMenu anchorRef rely on ref.current, passing a callback ref here can break at runtime. Consider using an internal object ref and wiring forwardedRef via useRefObjectAsForwardedRef/useMergedRefs.
This issue also appears on line 428 of the same file.
See below for a potential fix:
export const ActionBarGroup = forwardRef<HTMLDivElement, React.PropsWithChildren>(
({children}, forwardedRef) => {
const ref = useRef<HTMLDivElement>(null)
useRefObjectAsForwardedRef(forwardedRef, ref)
const {id, isOverflowing} = useActionBarItem(
ref,
useMemo((): ChildProps => ({type: 'group'}), []),
)
return (
<ActionBarGroupContext.Provider value={{groupId: id}}>
<div className={styles.Group} ref={ref} data-overflowing={isOverflowing} aria-hidden={isOverflowing}>
{children}
</div>
</ActionBarGroupContext.Provider>
)
},
)
export const ActionBarMenu = forwardRef<HTMLButtonElement, ActionBarMenuProps>(
(
{'aria-label': ariaLabel, icon, overflowIcon, items, returnFocusRef, ...props},
forwardedRef,
) => {
const ref = useRef<HTMLButtonElement>(null)
useRefObjectAsForwardedRef(forwardedRef, ref)
There was a problem hiding this comment.
I'm hoping to ship #7638 first and then use useCombinedRefs from there
francinelucca
left a comment
francinelucca
left a comment
There was a problem hiding this comment.
something's off here 👀
Screen.Recording.2026-03-11.at.3.14.32.PM.mov
Should be resolved now by setting |
Removed!
Yep, that's the same issue. It's (ab)using |
francinelucca
left a comment
francinelucca
left a comment
There was a problem hiding this comment.
these don't look correctly centered:
I'm still having this issue where once I resize one of the ActionBars becomes unreachable
Screen.Recording.2026-04-06.at.5.13.42.PM.mov
All other previous issues seem resolved
Created a bug to track -> https://github.com/github/primer/issues/6532 |
|
francinelucca
left a comment
francinelucca
left a comment
There was a problem hiding this comment.
1 - There's a new tab stop on the last element of the ActionBar now 🤔. Like if I'm not positioned in the last element (more menu needs to be visible), I have to tab twice to leave the ActionBar
Screen.Recording.2026-04-08.at.11.25.58.PM.mov
2 - When i'm on https://primer-c23a44cf2f-13348165.drafts.github.io/storybook/?path=/story/experimental-components-actionbar-examples--with-groups with all the items hidden (just the more button showing), I'm having a hard time actually getting there via keyboard, there seems to be no tab stop
iansan5653
requested review from
francinelucca
and removed request for
francinelucca
That's a really strange one. I wonder if it's a bug in
I can't repro this one - it seems to be working right for me? Screen.Recording.2026-04-13.at.10.08.43.AM.mov |
|
Confirming, can't repro the second one anymore 🤷🏽 |
|
Okay, I think I finally figured out why I'm pretty sure I was able to get everything working correctly by passing the overflow items array as a dependency. |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/19051 |
Takes the CSS flex wrapping approach to detecting overflowing from #7506 and applies it to
ActionBar, resulting in a much simpler and more reliable solution that is also SSR-stable:Screen.Recording.2026-03-11.at.2.37.39.PM.mov
Compare to the current implementation, which flickers on load and doesn't calculate the overflow as accurately, causing items to occasionally get clipped (this is more noticeable on production where it can sometimes calculate the overflow incorrectly; but that's hard to show in Storybook):
Screen.Recording.2026-03-11.at.2.42.41.PM.mov
ActionBaroverflow #7447Changelog
New
Changed
Improves
ActionBaroverflow calculations and SSR supportRemoved
Rollout strategy
Testing & Reviewing
Merge checklist