Re-add (unrevert) "Replace ActionBar overflow calculations with CSS wrapping approach"#7816
Re-add (unrevert) "Replace ActionBar overflow calculations with CSS wrapping approach"#7816iansan5653 wants to merge 9 commits into
Conversation
🦋 Changeset detectedLatest commit: 87ad0ca 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
Re-implements the earlier ActionBar overflow redesign by switching from width-based overflow calculations to a CSS-wrapping + JS observation approach, aiming to improve SSR stability and reduce flicker/clipping issues.
Changes:
- Replace ActionBar overflow detection with a flex-wrap/clip strategy and register overflowed items via
IntersectionObserver+useSyncExternalStore. - Update ActionBar styling to support wrapping/overflow clipping and conditionally reveal the “More” button.
- Adjust Storybook example and e2e assertions to better reproduce and validate the overflow scenario; add a changeset for a minor release.
Show a summary per file
| File | Description |
|---|---|
| packages/react/src/ActionBar/ActionBar.tsx | Replaces overflow logic with registry-driven overflow item detection and always-renders an overflow menu trigger. |
| packages/react/src/ActionBar/ActionBar.module.css | Adds wrapping/overflow clipping styles and scroll-timeline-based overflow detection to drive “More” button visibility. |
| packages/react/src/ActionBar/ActionBar.test.tsx | Updates a zero-width test to select the button by data-testid. |
| packages/react/src/ActionBar/ActionBar.examples.stories.tsx | Tweaks CommentBox layout to reproduce the overflow bug scenario more reliably. |
| packages/react/src/ActionBar/ActionBar.examples.stories.module.css | Updates CommentBox header styles to match the new reproduction scenario (incl. overflow constraints). |
| e2e/components/drafts/ActionBar.test.ts | Updates e2e assertions to count only visible IconButtons after overflow occurs. |
| .changeset/many-suns-promise.md | Declares a minor release for the ActionBar overflow behavior change. |
Copilot's findings
Comments suppressed due to low confidence (2)
packages/react/src/ActionBar/ActionBar.tsx:252
- The overflow “More” button accessible name is derived from
ariaLabeland appendsoverflowItems?.length. This can produce labels likeMore undefined items …when the ActionBar is labeled viaaria-labelledby, and it also changes the button’s accessible name as items overflow (breaking the e2e selector that looks forMore Comment box toolbar items). Use a stable label that doesn’t include dynamic counts and that works whenaria-labelledbyis used (e.g., fall back to a generic “More items”, or add a separatearia-labelledby/aria-describedbystrategy).
<IconButton
variant="invisible"
aria-label={`More ${ariaLabel} items ${overflowItems?.length}`}
icon={KebabHorizontalIcon}
className={styles.MoreButton}
data-more-button-inactive={overflowItems?.length ? undefined : true}
size={size}
/>
packages/react/src/ActionBar/ActionBar.tsx:213
overflowItemscurrently includes any registered descendant whose registry value is non-null, including{type: 'group'}entries fromActionBar.Group. Thosegroupentries are not handled in the overflow menu rendering (only divider/action/menu are), so they inflateoverflowItems.length(used fordata-has-overflowand the More button label) and can yieldundefinedchildren in theActionList. Filter outtype: 'group'here (or avoid registering the group container in the registry) sooverflowItemsonly contains actual renderable menu items.
const overflowItems = useMemo(
() =>
childRegistry &&
Array.from(childRegistry.entries()).filter((entry): entry is [string, ChildProps] => entry[1] !== null),
[childRegistry],
- Files reviewed: 7/7 changed files
- Comments generated: 2
iansan5653
added
the
Canary Release
|
@copilot this e2e test failure was likely introduced by the story changes in 560b2a8; investigate and resolve |
Agent-Logs-Url: https://github.com/primer/react/sessions/72cf5b58-6a9f-48d3-a3c8-f50e2ad0db11 Co-authored-by: iansan5653 <2294248+iansan5653@users.noreply.github.com>
Addressed in 30a5cc0. I took the minimal test-only path and narrowed the resize step to |
iansan5653
added
the
update snapshots
github-actions
Bot
removed
the
update snapshots
francinelucca
left a comment
francinelucca
left a comment
There was a problem hiding this comment.
Looking good but I think I found an error: looks like I can move the divider to the front by resizing 👀
Screen.Recording.2026-05-11.at.10.16.36.AM.mov
Looks like the fake 0-width first item which was supposed to allow the real first item (the first group) to wrap out of view was too short, so that the first item only wrapped down a couple of pixels. This was enough for the The fix is simple - just make the fake item fill the height of the container, so that it pushes other items fully out of view when they wrap. |
|
👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/20370 |
Re-implements #7655 after it was reverted in #7807, with fixes:
CommentBoxstory to reproduce the issue (it seems to have been caused by a particular set of container styles, most importantlyoverflow: hiddenon the containerIntersectionObserverthreshold from1to0.75so that it will fire reliably - the issue was caused by the observer not being quite sensitive enough; the buttons appear to have been still overflowing by just 1 pixel, which meant they were never considered 100% visibleThis reverts #7807.
➡️ View just the new changes