Skip to content

[Action Bar] Fix warning with menu items #6935

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pksjce merged 8 commits into from
Oct 8, 2025
Merged

[Action Bar] Fix warning with menu items #6935

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
efd81b8
Fix unique keys error
JelloBagel Oct 1, 2025
071c37e
Add header right to comment box story
JelloBagel Oct 1, 2025
4ba828b
add changeset
JelloBagel Oct 1, 2025
2bb06dc
Update e2e test for right header
JelloBagel Oct 1, 2025
685fc99
Update test for width
JelloBagel Oct 1, 2025
414854d
test(vrt): update snapshots
JelloBagel Oct 1, 2025
c24d35a
Merge branch 'main' into jellobagel-action-bar
pksjce Oct 8, 2025
4402b7a
Merge branch 'main' into jellobagel-action-bar
pksjce Oct 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/smart-hornets-happen.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Fix overflow calculations of more menu button in action bar
8 changes: 4 additions & 4 deletions e2e/components/drafts/ActionBar.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,15 +31,15 @@ test.describe('ActionBar', () => {
},
})
const toolbarButtonSelector = `button[data-component="IconButton"]`
await expect(page.locator(toolbarButtonSelector)).toHaveCount(9)
await expect(page.locator(toolbarButtonSelector)).toHaveCount(10)
await page.setViewportSize({width: viewports['primer.breakpoint.xs'], height: 768})
await page.getByLabel('Insert Link').waitFor({
await page.getByLabel('Task List').waitFor({
state: 'hidden',
})
await expect(page.locator(toolbarButtonSelector)).toHaveCount(5)
await expect(page.locator(toolbarButtonSelector)).toHaveCount(8)
const moreButtonSelector = page.getByLabel('More Comment box toolbar items')
await moreButtonSelector.click()
await expect(page.locator('ul[role="menu"] [role="menuitem"]')).toHaveCount(6)
await expect(page.locator('ul[role="menu"] [role="menuitem"]')).toHaveCount(3)
})
})
}
Expand Down
10 changes: 9 additions & 1 deletion packages/react/src/ActionBar/ActionBar.examples.stories.module.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,15 @@
}

.CommentBoxHeaderLeft {
width: 50%;
width: 100%;
min-width: 0;
}

.CommentBoxHeaderRight {
display: flex;
gap: var(--base-size-8);
justify-content: flex-end;
align-items: center;
}

.DialogContent {
Expand Down
4 changes: 4 additions & 0 deletions packages/react/src/ActionBar/ActionBar.examples.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,10 @@ export const CommentBox = (props: CommentBoxProps) => {
></ActionBar.IconButton>
</ActionBar>
</div>
<div className={classes.CommentBoxHeaderRight}>
<Button variant="invisible">Write</Button>
<Button variant="invisible">Preview</Button>
</div>
</header>
<Textarea value={value} onChange={e => setValue(e.target.value)} id="markdowninput" aria-label="Markdown value" />
<Dialog aria-labelledby="header" returnFocusRef={buttonRef} isOpen={isOpen} onDismiss={() => setIsOpen(false)}>
Expand Down
10 changes: 2 additions & 8 deletions packages/react/src/ActionBar/ActionBar.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -217,16 +217,10 @@ export const ActionBar: React.FC<React.PropsWithChildren<ActionBarProps>> = prop
if (menuItem.type === ActionList.Divider) {
return <ActionList.Divider key={index} />
} else {
const {
children: menuItemChildren,
onClick,
icon: Icon,
'aria-label': ariaLabel,
disabled,
} = menuItem.props
const {onClick, icon: Icon, 'aria-label': ariaLabel, disabled} = menuItem.props
return (
<ActionList.Item
key={menuItemChildren}
key={ariaLabel}

Copilot AI Oct 1, 2025

Copy link

Choose a reason for hiding this comment

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

Using ariaLabel as the key could cause issues if multiple menu items have the same aria-label or if aria-label is undefined. Consider using index or a combination of index and ariaLabel to ensure uniqueness.

Suggested change
key={ariaLabel}
key={`${index}-${ariaLabel ?? 'item'}`}

Copilot uses AI. Check for mistakes.

@pksjce pksjce Oct 1, 2025

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why not make it related to index ? Would index be changing between renders somehow?

@JelloBagel JelloBagel Oct 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The index could change if the window is resized. For example, if the toolbar overflowed at the "Insert Link" button, then it would be the first index in the menu and the key would be "0". When the window is resized smaller to overflow the next button, then the "Insert Code" button would be the first index now with a key of "0". While each key is still unique, this can cause odd issues if the keys match different components when rerendering in React

// eslint-disable-next-line primer-react/prefer-action-list-item-onselect
onClick={(event: React.MouseEvent<HTMLLIElement, MouseEvent>) => {
closeOverlay()
Expand Down
Loading