Skip to content

Re-add (unrevert) "Replace ActionBar overflow calculations with CSS wrapping approach"#7816

Open
iansan5653 wants to merge 7 commits intomainfrom
re-add-actionbar-css
Open

Re-add (unrevert) "Replace ActionBar overflow calculations with CSS wrapping approach"#7816
iansan5653 wants to merge 7 commits intomainfrom
re-add-actionbar-css

Conversation

@iansan5653
Copy link
Copy Markdown
Contributor

@iansan5653 iansan5653 commented May 7, 2026

Re-implements #7655 after it was reverted in #7807, with fixes:

  1. Updated the CommentBox story to reproduce the issue (it seems to have been caused by a particular set of container styles, most importantly overflow: hidden on the container
  2. Reduce the IntersectionObserver threshold from 1to 0.75 so 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% visible
  3. Remove a style condition to always apply the scroll driven animation and thus hopefully resolve some flickering (which just appeared to be caused by React safe mode, but I still wanted to resolve it)

This reverts #7807.

➡️ View just the new changes

Copilot AI review requested due to automatic review settings May 7, 2026 16:55
@iansan5653 iansan5653 requested a review from a team as a code owner May 7, 2026 16:55
@iansan5653 iansan5653 requested a review from TylerJDev May 7, 2026 16:55
@changeset-bot
Copy link
Copy Markdown

changeset-bot Bot commented May 7, 2026

🦋 Changeset detected

Latest commit: a825a29

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

This PR includes changesets to release 1 package
Name Type
@primer/react Minor

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 May 7, 2026
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented May 7, 2026

⚠️ Action required

👋 Hi, this pull request contains changes to the source code that github/github-ui depends on. If you are GitHub staff, test these changes with github/github-ui using the integration workflow. Check the integration testing docs for step-by-step instructions. Or, apply the integration-tests: skipped manually label to skip these checks.

To publish a canary release for integration testing, apply the Canary Release label to this PR.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 ariaLabel and appends overflowItems?.length. This can produce labels like More undefined items … when the ActionBar is labeled via aria-labelledby, and it also changes the button’s accessible name as items overflow (breaking the e2e selector that looks for More Comment box toolbar items). Use a stable label that doesn’t include dynamic counts and that works when aria-labelledby is used (e.g., fall back to a generic “More items”, or add a separate aria-labelledby/aria-describedby strategy).
              <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

  • overflowItems currently includes any registered descendant whose registry value is non-null, including {type: 'group'} entries from ActionBar.Group. Those group entries are not handled in the overflow menu rendering (only divider/action/menu are), so they inflate overflowItems.length (used for data-has-overflow and the More button label) and can yield undefined children in the ActionList. Filter out type: 'group' here (or avoid registering the group container in the registry) so overflowItems only 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

Comment thread packages/react/src/ActionBar/ActionBar.tsx
Comment thread packages/react/src/ActionBar/ActionBar.test.tsx
@github-actions github-actions Bot temporarily deployed to storybook-preview-7816 May 7, 2026 17:45 Inactive
@iansan5653 iansan5653 added the Canary Release Apply this label when you want CI to create a canary release of the current PR label May 7, 2026
@iansan5653
Copy link
Copy Markdown
Contributor Author

@copilot this e2e test failure was likely introduced by the story changes in 560b2a8; investigate and resolve

17) e2e/components/drafts/ActionBar.test.ts:42:13 › ActionBar › ActionBar Interactions › dark_colorblind › Overflow interaction @vrt 

    Test timeout of 15000ms exceeded.

    Error: locator.waitFor: Test timeout of 15000ms exceeded.
    Call log:
      - waiting for getByLabel('Task List') to be hidden
        34 × locator resolved to visible <button type="button" tabindex="-1" data-size="medium" data-loading="false" data-no-visuals="true" aria-labelledby=":rt:" data-variant="invisible" data-component="IconButton" class="prc-Button-ButtonBase-c50BI prc-Button-IconButton-szpyj">…</button>


      50 |           await expect(page.locator(toolbarButtonSelector).filter({visible: true})).toHaveCount(10)
      51 |           await page.setViewportSize({width: viewports['primer.breakpoint.xs'], height: 768})
    > 52 |           await page.getByLabel('Task List').waitFor({
         |                                              ^
      53 |             state: 'hidden',
      54 |           })
      55 |           await expect(page.locator(toolbarButtonSelector).filter({visible: true})).toHaveCount(8)
        at /github/workspace/e2e/components/drafts/ActionBar.test.ts:52:46

@primer-integration
Copy link
Copy Markdown

👋 Hi from github/github-ui! Your integration PR is ready: https://github.com/github/github-ui/pull/20186

@primer-integration
Copy link
Copy Markdown

Integration test results from github/github-ui:

Running  CI   Running
Passed  VRT   Passed
Passed  Projects   Passed

@primer-integration
Copy link
Copy Markdown

Integration test results from github/github-ui:

Passed  CI   Passed
Passed  VRT   Passed
Passed  Projects   Passed

All checks passed!

Agent-Logs-Url: https://github.com/primer/react/sessions/72cf5b58-6a9f-48d3-a3c8-f50e2ad0db11

Co-authored-by: iansan5653 <2294248+iansan5653@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI commented May 7, 2026

@copilot this e2e test failure was likely introduced by the story changes in 560b2a8; investigate and resolve

17) e2e/components/drafts/ActionBar.test.ts:42:13 › ActionBar › ActionBar Intera...

Addressed in 30a5cc0. I took the minimal test-only path and narrowed the resize step to viewports['primer.breakpoint.xs'] - 64 (480px), which matches the updated CommentBox story layout and reliably moves Task List into the overflow menu again. I re-ran CI= npx playwright test e2e/components/drafts/ActionBar.test.ts --grep 'Overflow interaction' and all 9 themed cases passed locally. Screenshot: /tmp/actionbar-overflow-480.png

@francinelucca francinelucca self-requested a review May 8, 2026 02:28
@github-actions github-actions Bot temporarily deployed to storybook-preview-7816 May 8, 2026 16:53 Inactive
@iansan5653 iansan5653 added the update snapshots 🤖 Command that updates VRT snapshots on the pull request label May 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Canary Release Apply this label when you want CI to create a canary release of the current PR 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.

3 participants