Skip to content

Commit bbbc4b3

Browse files
rezrahrezrah
authored
Fix ActionMenu bugs (#1401)
* fix actionmenu bug reports * update docs
1 parent 5a29cf5 commit bbbc4b3

9 files changed

Lines changed: 257 additions & 21 deletions

‎.changeset/fix-action-menu-split-button-click.md‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
---
2+
'@primer/react-brand': minor
3+
---
4+
5+
Multiple `ActionMenu` component updates:
6+
7+
- Calls consumer-provided `onClick` handlers in default and split-button modes.
8+
- ⚠️ Breaking change to `ActionMenu.Button` in split-button mode. Custom HTML attributes are now correctly forwarded to the primary action instead of the menu toggle. This was the originally intended behavior.
9+
- Forwards the `variant` prop in `default` mode correctly. `ActionMenu` can now render in all available `Button` variants.
10+
- Forwards the `leadingVisual` prop in `default` mode.

‎apps/next-docs/content/components/ActionMenu/index.mdx‎

Lines changed: 9 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ import {
1919
ActionMenuMenuAlignmentProp,
2020
ActionMenuMenuSideProp,
2121
ActionMenuModeProp,
22+
ActionMenuButtonVariantsProp,
2223
} from './react'
2324

2425
```js
@@ -77,11 +78,6 @@ render(<App />)
7778

7879
In this `mode`, the `ActionMenu` can be shown as a split button with an action (left) and dropdown button (right) with an additional list of actions.
7980

80-
<Note>
81-
In split-button mode, each action must be rendered as a link element using the as="a" prop, including the main button
82-
and all menu items.
83-
</Note>
84-
8581
```jsx live
8682
<ActionMenu mode="split-button">
8783
<ActionMenu.Button variant="subtle" as="a" href="#location-for-link" leadingVisual={<StarIcon />}>
@@ -144,11 +140,14 @@ In this `mode`, the `ActionMenu` can be shown as a split button with an action (
144140

145141
### ActionMenu.Button <Label>Required</Label>
146142

147-
| name | type | default | required | description |
148-
| ----------- | -------------- | ------- | -------- | ---------------------------------- |
149-
| `className` | `string` | | | Sets a custom class on the element |
150-
| `children` | `ReactElement` | | |
151-
| `id` | `string` | | | Sets a custom `id` |
143+
| name | type | default | required | description |
144+
| --------------- | -------------------------------- | ----------- | -------- | ------------------------------------------------ |
145+
| `className` | `string` | | | Sets a custom class on the element |
146+
| `children` | `ReactElement` | | | |
147+
| `id` | `string` | | | Sets a custom `id` |
148+
| `variant` | <ActionMenuButtonVariantsProp /> | | `false` | Sets the button styling variation |
149+
| `onClick` | `(event: MouseEvent) => void` | `undefined` | `false` | Handles activation of the button |
150+
| `leadingVisual` | `ReactElement` | `undefined` | `false` | Renders a leading visual before the button label |
152151

153152
### ActionMenu.Overlay <Label>Required</Label>
154153

‎apps/next-docs/content/components/ActionMenu/react.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ import {ActionMenuButtonModes} from '../../../../../packages/react/src/ActionMen
66
export const ActionMenuSizesProp = () => <PropTableValues values={['small', 'medium']} commaSeparated />
77
export const ActionMenuSelectionVariantProp = () => <PropTableValues values={['single', 'none']} commaSeparated />
88
export const ActionMenuMenuAlignmentProp = () => <PropTableValues values={['start', 'end']} commaSeparated />
9+
export const ActionMenuButtonVariantsProp = () => (
10+
<PropTableValues values={['primary', 'secondary', 'subtle']} commaSeparated />
11+
)
912

1013
export const ActionMenuMenuSideProp = () => (
1114
<PropTableValues

‎packages/react/src/ActionMenu/ActionMenu.features.stories.tsx‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,36 @@ export const SingleSelectionSmallOpen = () => {
6161
)
6262
}
6363

64+
export const DefaultModeAllVariants = () => {
65+
const {t} = useTranslation('ActionMenu')
66+
67+
return (
68+
<Stack direction="horizontal" gap="condensed">
69+
{ButtonVariants.map(variant => (
70+
<ActionMenu key={variant}>
71+
<ActionMenu.Button variant={variant}>{t('open_menu')}</ActionMenu.Button>
72+
<ActionMenu.Overlay aria-label={t('actions')}>
73+
<ActionMenu.Item value="Copy link">{t('copy_link')}</ActionMenu.Item>
74+
</ActionMenu.Overlay>
75+
</ActionMenu>
76+
))}
77+
</Stack>
78+
)
79+
}
80+
81+
export const DefaultModeLeadingVisual = () => {
82+
const {t} = useTranslation('ActionMenu')
83+
84+
return (
85+
<ActionMenu>
86+
<ActionMenu.Button leadingVisual={<VisualStudioCodeLogo />}>{t('open_menu')}</ActionMenu.Button>
87+
<ActionMenu.Overlay aria-label={t('actions')}>
88+
<ActionMenu.Item value="Copy link">{t('copy_link')}</ActionMenu.Item>
89+
</ActionMenu.Overlay>
90+
</ActionMenu>
91+
)
92+
}
93+
6494
export const SplitButtonMode = () => {
6595
const {t} = useTranslation('ActionMenu')
6696

‎packages/react/src/ActionMenu/ActionMenu.test.tsx‎

Lines changed: 146 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import React, {useState} from 'react'
22
import {render, cleanup, fireEvent, waitFor} from '@testing-library/react'
33
import '@testing-library/jest-dom'
4+
import userEvent from '@testing-library/user-event'
45
import {axe, toHaveNoViolations} from 'jest-axe'
56

67
import {ActionMenu} from './ActionMenu'
@@ -111,6 +112,81 @@ describe('ActionMenu', () => {
111112
)
112113
})
113114

115+
it("should forward the user's onClick callback and toggle the menu", () => {
116+
const mockOnClick = jest.fn()
117+
const {getByRole, queryByLabelText} = render(
118+
<ActionMenu>
119+
<ActionMenu.Button onClick={mockOnClick}>Open menu</ActionMenu.Button>
120+
<ActionMenu.Overlay aria-label="Actions">
121+
<ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
122+
</ActionMenu.Overlay>
123+
</ActionMenu>,
124+
)
125+
126+
fireEvent.click(getByRole('button', {name: 'Open menu'}))
127+
128+
expect(mockOnClick).toHaveBeenCalledTimes(1)
129+
expect(queryByLabelText('Actions')).toBeInTheDocument()
130+
})
131+
132+
it("should not toggle the menu when the user's onClick callback prevents the default action", () => {
133+
const mockOnClick = jest.fn(event => event.preventDefault())
134+
const {getByRole, queryByLabelText} = render(
135+
<ActionMenu>
136+
<ActionMenu.Button onClick={mockOnClick}>Open menu</ActionMenu.Button>
137+
<ActionMenu.Overlay aria-label="Actions">
138+
<ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
139+
</ActionMenu.Overlay>
140+
</ActionMenu>,
141+
)
142+
143+
fireEvent.click(getByRole('button', {name: 'Open menu'}))
144+
145+
expect(mockOnClick).toHaveBeenCalledTimes(1)
146+
expect(queryByLabelText('Actions')).not.toBeInTheDocument()
147+
})
148+
149+
it('should apply the button variant in default mode', () => {
150+
const {getByRole} = render(
151+
<ActionMenu>
152+
<ActionMenu.Button variant="subtle">Open menu</ActionMenu.Button>
153+
<ActionMenu.Overlay aria-label="Actions">
154+
<ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
155+
</ActionMenu.Overlay>
156+
</ActionMenu>,
157+
)
158+
159+
expect(getByRole('button', {name: 'Open menu'})).toHaveClass('Button--subtle')
160+
})
161+
162+
it('should use the secondary button variant by default in default mode', () => {
163+
const {getByRole} = render(
164+
<ActionMenu>
165+
<ActionMenu.Button>Open menu</ActionMenu.Button>
166+
<ActionMenu.Overlay aria-label="Actions">
167+
<ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
168+
</ActionMenu.Overlay>
169+
</ActionMenu>,
170+
)
171+
172+
expect(getByRole('button', {name: 'Open menu'})).toHaveClass('Button--secondary')
173+
})
174+
175+
it('should render the leading visual in default mode', () => {
176+
const accessibleText = 'Test icon'
177+
const TestIcon = () => <svg aria-label={accessibleText} />
178+
const {getByLabelText} = render(
179+
<ActionMenu>
180+
<ActionMenu.Button leadingVisual={<TestIcon />}>Open menu</ActionMenu.Button>
181+
<ActionMenu.Overlay aria-label="Actions">
182+
<ActionMenu.Item value="Copy link">Copy link</ActionMenu.Item>
183+
</ActionMenu.Overlay>
184+
</ActionMenu>,
185+
)
186+
187+
expect(getByLabelText(accessibleText)).toBeInTheDocument()
188+
})
189+
114190
it("should set aria-haspopup to 'true' and aria-expanded to 'false' by default", () => {
115191
const {getByRole} = render(
116192
<ActionMenu onSelect={jest.fn()}>
@@ -455,6 +531,24 @@ describe('ActionMenu', () => {
455531
expect(mainButton).toHaveAttribute('href', '#option1')
456532
})
457533

534+
it('should forward custom attributes to the primary action in split-button mode', () => {
535+
const {getByRole, getByLabelText} = render(
536+
<ActionMenu mode="split-button">
537+
<ActionMenu.Button as="a" href="#option1" data-attribute="test">
538+
Primary Action
539+
</ActionMenu.Button>
540+
<ActionMenu.Overlay aria-label="Additional options">
541+
<ActionMenu.Item as="a" href="#option1">
542+
Option 1
543+
</ActionMenu.Item>
544+
</ActionMenu.Overlay>
545+
</ActionMenu>,
546+
)
547+
548+
expect(getByRole('link', {name: 'Primary Action'})).toHaveAttribute('data-attribute', 'test')
549+
expect(getByLabelText('Menu')).not.toHaveAttribute('data-attribute')
550+
})
551+
458552
it('should toggle menu when clicking the chevron button', async () => {
459553
const {getByLabelText, queryByLabelText} = render(
460554
<ActionMenu mode="split-button">
@@ -501,6 +595,40 @@ describe('ActionMenu', () => {
501595
)
502596
})
503597

598+
it('should keep the primary and menu actions independent in split-button mode', async () => {
599+
const mockOnClick = jest.fn()
600+
const user = userEvent.setup()
601+
const {getByRole, getByLabelText, queryByLabelText} = render(
602+
<ActionMenu mode="split-button">
603+
<ActionMenu.Button onClick={mockOnClick}>Primary Action</ActionMenu.Button>
604+
<ActionMenu.Overlay aria-label="Additional options">
605+
<ActionMenu.Item as="a" href="#option1">
606+
Option 1
607+
</ActionMenu.Item>
608+
</ActionMenu.Overlay>
609+
</ActionMenu>,
610+
)
611+
612+
const primaryButton = getByRole('button', {name: 'Primary Action'})
613+
fireEvent.click(primaryButton)
614+
615+
expect(mockOnClick).toHaveBeenCalledTimes(1)
616+
expect(queryByLabelText('Additional options')).not.toBeInTheDocument()
617+
618+
await user.tab()
619+
expect(primaryButton).toHaveFocus()
620+
await user.keyboard('{Enter}')
621+
await user.keyboard(' ')
622+
623+
expect(mockOnClick).toHaveBeenCalledTimes(3)
624+
expect(queryByLabelText('Additional options')).not.toBeInTheDocument()
625+
626+
fireEvent.click(getByLabelText('Menu'))
627+
628+
expect(mockOnClick).toHaveBeenCalledTimes(3)
629+
expect(queryByLabelText('Additional options')).toBeInTheDocument()
630+
})
631+
504632
it('should render items as links with correct href attribute', async () => {
505633
const {getByLabelText, getAllByRole} = render(
506634
<ActionMenu mode="split-button" open>
@@ -574,6 +702,24 @@ describe('ActionMenu', () => {
574702
expect(variantButton).toBeInTheDocument()
575703
})
576704

705+
it('should use the primary button variant by default in split-button mode', () => {
706+
const {getByRole, getByLabelText} = render(
707+
<ActionMenu mode="split-button">
708+
<ActionMenu.Button as="a" href="#option1">
709+
Primary Action
710+
</ActionMenu.Button>
711+
<ActionMenu.Overlay aria-label="Additional options">
712+
<ActionMenu.Item as="a" href="#option1">
713+
Option 1
714+
</ActionMenu.Item>
715+
</ActionMenu.Overlay>
716+
</ActionMenu>,
717+
)
718+
719+
expect(getByRole('link', {name: 'Primary Action'})).toHaveClass('Button--primary')
720+
expect(getByLabelText('Menu')).toHaveClass('Button--primary')
721+
})
722+
577723
it('should not change main button href when menu is toggled', async () => {
578724
const {getByText, getByLabelText} = render(
579725
<ActionMenu mode="split-button">

0 commit comments

Comments
 (0)