Replace ActionBar overflow calculations with CSS wrapping approach#7655
Replace ActionBar overflow calculations with CSS wrapping approach#7655iansan5653 wants to merge 25 commits intomainfrom
ActionBar overflow calculations with CSS wrapping approach#7655Conversation
🦋 Changeset detectedLatest commit: d533eb5 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 |
|
🤖 Lint and formatting issues have been automatically fixed and committed to this PR. |
francinelucca
left a comment
francinelucca
left a comment
There was a problem hiding this comment.
Getting closer, found a few issues:
- it looks like the KeyBindingHint story isn't working properly:
Screen.Recording.2026-04-02.at.10.28.33.PM.mov
- Something weird is going on with resizing the screen in this story. I can't get back to one of the ActionBars via keyboard:
Screen.Recording.2026-04-02.at.10.39.26.PM.mov
- Focus state looks clipped in small ActionBar story
Screen.Recording.2026-04-02.at.10.43.37.PM.mov
- These are disappearing
Screen.Recording.2026-04-02.at.10.45.17.PM.mov
- When this story is wrapped all the way I can't get to it via keyboard
Screen.Recording.2026-04-02.at.10.49.03.PM.mov
| ActionBarItemsRegistry.useRegisterDescendant(isOverflowing ? registryProps : null) | ||
|
|
||
| return width | ||
| return {isOverflowing, dataOverflowingAttr: isOverflowing ? '' : undefined} |
There was a problem hiding this comment.
does this actually need to return the dataOverflowingAttr? couldn't we just look at the value of isOverflowing to set that on consumer side? wondering why both are needed here 🤔
There was a problem hiding this comment.
Yeah it totally could and that was how I originally had it. But we'd have to do it for every different kind of item, so it seemed to make sense to just pull the common logic out to here and ensure we are consistent. This is an internal (not consumer facing) hook, so I wasn't too worried about the API being a little redundant
| } | ||
| return breakpoint | ||
| } | ||
| const ActionBarItemsRegistry = createDescendantRegistry<ChildProps | null>() |
There was a problem hiding this comment.
does this need to take in the ChildProps? if I'm reading correctly the registry is only read on line 240 to check if there's a non-null value? wondering if this can be simplified if so 🤔
There was a problem hiding this comment.
We do need the props because we also use it in L238-240 where we build the list of items for the overflow menu. The props tells us what label to render, what to do on click, etc
|
Thanks for the thorough review! Looks like tooltips in general aren't working on toolbar items 🤔. I'll dig in to that; maybe they are getting clipped or hidden.
For this one, it doesn't seem like there's really a well-defined preferred behavior here? These buttons aren't I think what we should do is just implement an |
You can still scroll to them vs in this implementation they're completely gone so idk about worse.
I agree this seems like an unsupported case. I almost want to remove that example entirely until this is officially supported by ActionBar. CC @TylerJDev I see you introduced the story in #5476, do you remember if there was a reason/use-case for this? want to make sure we don't break anything if we move forward |
That also makes sense to me 👍 FWIW it wouldn't be difficult at all to add support for text buttons, especially with the new approach. I could plan to do it in a followup PR. |
|
Okay, turns out the tooltip issue repros in Still working on the other issues:
Could you please explain in more detail what the issue is here? I can't figure out what the bug is from your screencap.
Looks like I need to fix the overflow button size when the
Can repro this one; looking in to it. Update: resolved ✔️ |
I can't remember the context, so I think it's okay to remove! I don't think there's any real world cases that use ActionBar like this. I did notice one issue in github-ui the staffbar component renders differently in https://github.com/github/github-ui/pull/17750 compared to production storybook when you resize it horizontally (< 500px width). It looks like the items are offscreen 🤔 |
Removed!
Yep, that's the same issue. It's (ab)using |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/17853 |
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