Skip to content

fix(ButtonGroup): add toolbar interactions for role toolbar #5200

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.

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

5 changes: 5 additions & 0 deletions .changeset/slow-news-love.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@primer/react": patch
---

fix(ButtonGroup): add toolbar interactions for role toolbar
28 changes: 28 additions & 0 deletions e2e/components/ButtonGroup.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -86,4 +86,32 @@ test.describe('ButtonGroup', () => {
})
}
})

test.describe('As Toolbar', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-buttongroup-features--as-toolbar',
globals: {
colorScheme: theme,
},
})

// Default state
expect(await page.screenshot()).toMatchSnapshot(`ButtonGroup.As Toolbar.${theme}.png`)
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-buttongroup-features--as-toolbar',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations()
})
})
}
})
})
Original file line number Diff line number Diff line change
Expand Up @@ -73,3 +73,11 @@ export const DropdownSplit = () => {
</ButtonGroup>
)
}

export const AsToolbar = () => (
<ButtonGroup role="toolbar">
<Button>Button 1</Button>
<Button>Button 2</Button>
<Button>Button 3</Button>
</ButtonGroup>
)
58 changes: 58 additions & 0 deletions packages/react/src/ButtonGroup/ButtonGroup.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
import {Button} from '../Button'
import {render, screen, fireEvent} from '@testing-library/react'

Check failure on line 2 in packages/react/src/ButtonGroup/ButtonGroup.test.tsx

View workflow job for this annotation

GitHub Actions / lint

'fireEvent' is defined but never used
import axe from 'axe-core'
import {FeatureFlags} from '../FeatureFlags'
import {behavesAsComponent} from '../utils/testing'
import type {ButtonGroupProps} from './ButtonGroup'
import ButtonGroup from './ButtonGroup'
import React from 'react'

const TestButtonGroup = (props: ButtonGroupProps) => (
<ButtonGroup {...props}>
<Button>Button 1</Button>
<Button>Button 2</Button>
<Button>Button 3</Button>
</ButtonGroup>
)

describe('ButtonGroup', () => {
behavesAsComponent({
Component: TestButtonGroup,
options: {skipSx: true, skipAs: true},
})

it('should support `className` on the outermost element', () => {
const Element = () => <ButtonGroup className={'test-class-name'} />
const FeatureFlagElement = () => {
return (
<FeatureFlags
flags={{
primer_react_css_modules_team: true,
primer_react_css_modules_staff: true,
primer_react_css_modules_ga: true,
}}
>
<Element />
</FeatureFlags>
)
}
expect(render(<Element />).container.firstChild).toHaveClass('test-class-name')
expect(render(<FeatureFlagElement />).container.firstChild).toHaveClass('test-class-name')
})

it('renders a <div>', () => {
const container = render(<ButtonGroup data-testid="button-group" />)
expect(container.getByTestId('button-group').tagName).toBe('DIV')
})

it('should have no axe violations', async () => {
const {container} = render(<TestButtonGroup />)
const results = await axe.run(container)
expect(results).toHaveNoViolations()
})

it('should respect role prop', () => {
render(<ButtonGroup role="toolbar" />)
expect(screen.getByRole('toolbar')).toBeInTheDocument()
})
})
26 changes: 22 additions & 4 deletions packages/react/src/ButtonGroup/ButtonGroup.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ import classes from './ButtonGroup.module.css'
import {toggleStyledComponent} from '../internal/utils/toggleStyledComponent'
import {clsx} from 'clsx'
import {useFeatureFlag} from '../FeatureFlags'
import {FocusKeys, useFocusZone, type FocusZoneHookSettings} from '../hooks/useFocusZone'
import {useProvidedRefOrCreate} from '../hooks'
import type {ForwardRefComponent as PolymorphicForwardRefComponent} from '../utils/polymorphic'

const StyledButtonGroup = toggleStyledComponent(
'primer_react_css_modules_team',
Expand Down Expand Up @@ -73,24 +76,39 @@ const StyledButtonGroup = toggleStyledComponent(
`,
)

export type ButtonGroupProps = ComponentProps<typeof StyledButtonGroup>
export type ButtonGroupProps = ComponentProps<typeof StyledButtonGroup> & {
/**
* Settings to apply to the Focus Zone on the ButtonGroup container component. This is only used when role="toolbar" is supplied.
*/
focusZoneSettings?: Partial<FocusZoneHookSettings>
Copy link
Member

Choose a reason for hiding this comment

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

Would there be a way to capture relevant settings people might do as props on the component, instead? Just to avoid coupling this to focusZoneSettings if we want to refactor down the line.

Copy link
Member

Choose a reason for hiding this comment

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

We could potentially forgo the focusZoneSettings prop since this is well scoped for only applying if role="toolbar" is true. Since the expectations are clear on how role="toolbar" should operate with keyboard, I think it's okay if consumers aren't able to pass their own settings.

I'm thinking if they truly needed to utilize their own settings, they could just apply their own useFocusZone outside of the component, as it should replace the one that already exists, but I haven't tested so I'm not sure if it's true 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

Did a quick test and Tyler's right, useFocusZone in outer component will override behavior, removed focusZoneSettings here e8782e2. Thanks for the suggestion both!

}
const ButtonGroup = React.forwardRef<HTMLElement, ButtonGroupProps>(function ButtonGroup(
{children, className, ...rest},
{children, className, role, focusZoneSettings, ...rest},
forwardRef,
) {
const enabled = useFeatureFlag('primer_react_css_modules_team')
const buttonRef = useProvidedRefOrCreate(forwardRef as React.RefObject<HTMLDivElement>)

useFocusZone({
Copy link
Member

Choose a reason for hiding this comment

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

Could we add focusOutBehavior: 'wrap' to the object so that it wraps when you arrow the first/last item?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done in e8782e2! Thanks so much for the suggestion 🙏🏼

containerRef: buttonRef,
disabled: role !== 'toolbar',
bindKeys: FocusKeys.ArrowHorizontal,
...focusZoneSettings,
})

return (
<StyledButtonGroup
ref={forwardRef}
ref={buttonRef}
className={clsx(className, {
[classes.ButtonGroup]: enabled,
})}
role={role}
{...rest}
>
{children}
</StyledButtonGroup>
)
})
}) as PolymorphicForwardRefComponent<'div', ButtonGroupProps>
Copy link
Member Author

Choose a reason for hiding this comment

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

Had to add this because the tests where complaining about types when trying to spread ({...props}) of type ButtonGroupProps 🤷🏽‍♀️


ButtonGroup.displayName = 'ButtonGroup'

Expand Down
Loading