-
Notifications
You must be signed in to change notification settings - Fork 622
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
Changes from 2 commits
f959845
d904746
2309a2d
ccbe993
b51c79a
fb53032
7bd33b6
e8782e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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 |
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' | ||
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() | ||
}) | ||
}) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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', | ||
|
@@ -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> | ||
} | ||
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({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we add There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 ( |
||
|
||
ButtonGroup.displayName = 'ButtonGroup' | ||
|
||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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 ifrole="toolbar"
is true. Since the expectations are clear on howrole="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 😅There was a problem hiding this comment.
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, removedfocusZoneSettings
here e8782e2. Thanks for the suggestion both!