-
Notifications
You must be signed in to change notification settings - Fork 860
[EuiButtonGroupButton] Make isSelected prop optional
#9164
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
Conversation
so that aria-pressed can be omitted if needed, which enables button group usage different than selection-only
ea1fdde to
47ea284
Compare
💚 Build SucceededHistory
cc @acstll |
💚 Build Succeeded
History
cc @acstll |
weronikaolejniczak
left a comment
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.
Given that EuiButtonGroup is already passing a boolean and EuiButtonGroupButton now supports both standard and toggle behavior, the changes are correct 👍🏻
I verified that EuiButtonGroup works as expected:
| Before | After | |
|---|---|---|
| Windows + Edge + NVDA | Kapture.2025-11-03.at.12.07.52.mp4 |
Kapture.2025-11-03.at.12.09.02.mp4 |
| MacOS + Safari + VoiceOver | Kapture.2025-11-03.at.12.32.28.mp4 |
Kapture.2025-11-03.at.12.33.50.mp4 |
I also went ahead and used EuiButtonGroupButton as a regular button without isSelected and can verify aria-pressed is not present:
Thanks for the improvement, Arturo! 🙏🏻 🟢
Note: In the case were a component relied on EuiButtonGroupButton providing the false default, there could be an a11y regression. aria-pressed will work only if it's a boolean value, otherwise the screen reader gets confused and doesn't pronounce undefined as an "off" state of the toggle button. BUT EuiButtonGroupButton is only used internally in EuiButtonGroup and EuiFilterGroup and it works well.
|
@weronikaolejniczak thanks for taking the time to do the testing, much appreciated! |
Summary
EuiButtonGroupis currently meant "for indicating selection only" (docs). But button groups could be used to handle independent intent (clicks), as in toolbars e.g. upload a file.Important
This PR does not affect
EuiButtonGroup, only the internalEuiButtonGroupButton.The PR makes the
isSelectedprop optional, so that thearia-pressedattribute is not always present as to indicate selection. Allowing different usages of buttons within button groups.Why are we making this change?
To enable other usages for
EuiButtonGroupButtonother than selection.Needed to improve accessibility in #9151.
Impact to users
🟢 No impact, internal change.
QA
EuiButtonGroupis not affected and works as before.Remove or strikethrough items that do not apply to your PR.
General checklist
Checked in both light and dark modesChecked in both MacOS and Windows high contrast modes(emulate forced colors if you do not have access to a Windows machine.)Checked in mobileChecked in Chrome, Safari, Edge, and FirefoxChecked for accessibility including keyboard-only and screenreader modesAdded documentationProps have proper autodocs (using@defaultif default values are missing) and playground togglesChecked Code Sandbox works for any docs examplesUpdated visual regression testsIf applicable, added the breaking change issue label (and filled out the breaking change checklist)If the changes unblock an issue in a different repo, smoke tested carefully (see Testing EUI features in Kibana ahead of time)If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)