fix(ButtonGroup): add toolbar interactions for role toolbar #5200
Conversation
🦋 Changeset detectedLatest commit: e8782e2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
francinelucca
added
staff
|
👋 Hi, this pull request contains changes to the source code that github/github depends on. If you are GitHub staff, we recommend testing these changes with github/github using the integration workflow. Thanks! |
github-actions
bot
added
the
integration-tests: recommended
size-limit report 📦
|
francinelucca
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| </StyledButtonGroup> | ||
| ) | ||
| }) | ||
| }) as PolymorphicForwardRefComponent<'div', ButtonGroupProps> |
There was a problem hiding this comment.
Had to add this because the tests where complaining about types when trying to spread ({...props}) of type ButtonGroupProps 🤷🏽♀️
…-a-role--does-not-convey-its-purpose' of github.com:primer/react into francinelucca/3423-prcbuttongroup-group-container-lacks-a-role--does-not-convey-its-purpose
francinelucca
temporarily deployed
to
github-pages
GitHub Actions
Inactive
francinelucca
added
the
update snapshots
|
👋 Hi from github/github! Your integration PR is ready: https://github.com/github/github/pull/349486 |
github-actions
bot
removed
the
update snapshots
|
🟢 golden-jobs completed with status |
|
Uh oh! @francinelucca, the image you shared is missing helpful alt text. Check #5200 (comment). Alt text is an invisible description that helps screen readers describe images to blind or low-vision users. If you are using markdown to display images, add your alt text inside the brackets of the markdown image. Learn more about alt text at Basic writing and formatting syntax: images on GitHub Docs.
|
francinelucca
temporarily deployed
to
github-pages
GitHub Actions
Inactive
francinelucca
added
integration-tests: passing
…ainer-lacks-a-role--does-not-convey-its-purpose
francinelucca
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| /** | ||
| * Settings to apply to the Focus Zone on the ButtonGroup container component. This is only used when role="toolbar" is supplied. | ||
| */ | ||
| focusZoneSettings?: Partial<FocusZoneHookSettings> |
There was a problem hiding this comment.
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.
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 😅
There was a problem hiding this comment.
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 enabled = useFeatureFlag('primer_react_css_modules_team') | ||
| const buttonRef = useProvidedRefOrCreate(forwardRef as React.RefObject<HTMLDivElement>) | ||
|
|
||
| useFocusZone({ |
There was a problem hiding this comment.
Could we add focusOutBehavior: 'wrap' to the object so that it wraps when you arrow the first/last item?
There was a problem hiding this comment.
Done in e8782e2! Thanks so much for the suggestion 🙏🏼
…3-prcbuttongroup-group-container-lacks-a-role--does-not-convey-its-purpose
francinelucca
temporarily deployed
to
github-pages
GitHub Actions
Inactive
|
Probably a noob question, but do we anticipate any overlap with ActionBar, which is also a role="toolbar" with buttons and does that change our recommendation? Both options have no usage till now, ButtonGroup with role=toolbar has 0 instances, experimental/ActionBar also has 0 instances |
siddharthkp
left a comment
siddharthkp
left a comment
There was a problem hiding this comment.
Implementation looks good!
I think it's fine to have both functionalities in the codebase, we're not making toolbar the default behavior for Tagging @primer/design-reviewers in case there's any thoughts |
francinelucca
deleted the
francinelucca/3423-prcbuttongroup-group-container-lacks-a-role--does-not-convey-its-purpose
branch
Closes https://github.com/github/primer/issues/3423
Introduces an accessibility improvement to
ButtonGroupby adding correct toolbar interactions (navigate with left/right arrow instead of tab) when supplied role="toolbar".Changelog
New
ButtonGroupButtonGroup"As Toolbar" storyButtonGroup"As Toolbar" storyChanged
ButtonGroupasPolymorphicForwardRefComponent<'div', ButtonGroupProps>ButtonGroupto usefocusZonewith left/right arrow whenrole="toolbar"is supplied.Rollout strategy
Testing & Reviewing
Test deployed As Toolbar story, notice keyboard interaction. Compare to other ButtonGroup stories (these should have normal tab keyboard navigation)
Merge checklist