Fix EuiButtonGroup a11y and remove EuiToggle/EuiButtonToggle#4056
Fix EuiButtonGroup a11y and remove EuiToggle/EuiButtonToggle#4056cchaos merged 36 commits intoelastic:masterfrom
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4056/ |
and other tests
c8896f0 to
5edd2fc
Compare
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4056/ |
And using it in EuiButtonEmpty and EuiButtonIcon
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4056/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4056/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4056/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4056/ |
myasonik
left a comment
There was a problem hiding this comment.
a11y seems great, docs seem clear, 🚀 ship it!
One possible enhancement (can be ignored, added now, or added in a future issue): of isSelected is passed in, should that change any default styles? Could adjust fill state for you so that consumers don't have to pass in two props of the same value.
I had thought of this too, but that's not typically how we want the fill visual to indicate. Usually, |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4056/ |
|
jenkins test this |
|
If we're relying on the documentation to communicate this functionality, I'm not sure it's worth having the custom |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4056/ |
|
Well actually with the addition of the Also, this prop name aligns with the EuiButtonGroup's selection prop of The other nice thing is that it gives us a scalable prop to add more functionality as a "toggle" button if we need it in the future. |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4056/ |
chandlerprall
left a comment
There was a problem hiding this comment.
Changes LGTM; pulled & tested locally, including keyboard nav & screen reader support
…#4056) * [EuiButton] add `minWidth` prop * Adding text shift to button and fix DataGrid usage and other tests * [Breaking] Removing EuiToggle * [Breaking] Removed EuiButtonToggle in favor of EuiButton.isSelected
Replacement for #3099 and closes #3023
... and closes #4036
The EuiToggle component wasn't appropriately handling the accessibility of how a toggle should behave. It was replaced in EUI components with correct implementations and there's little to none usages outside of EUI, so it is safe to remove.
This PR also removes the EuiButtonToggle because it was using the EuiToggle component. Instead, we now support a quick addition of the appropriate
aria-pressedvia theisSelectedprop on all our button components.Upgrade path
labelto be thechildren.iconOnly, simply swap to using an EuiButtonIcon.isEmpty, simply swap to using an EuiButtonEmpty.All button replacements should address the toggle functionality in one of the two ways listed above:
aria-label, then there is no additional accessibility concern.aria-pressedpassing a boolean for the on and off states. All 3 button types (will) provides a helper prop for this calledisSelected.Re-built the entire EuiButtonGroup component
The two main concepts brought over from #3099 are:
1. Use a
<button aria-pressed="true or false">for multiple selection groupsThis is the preferred method for signifying whether a single button is on or off (pressed). Multi-selection groups are just a visually grouped button set all with their own state but under one legend.
2. Use a
<input type="radio">for single selection groupsSince only one button can be selected at a time for single selection groups, we make these behave as radio groups by actually using hidden radio groups. However, instead of trying to sync selection of a hidden input with the display, the element rendered by EuiButtonDisplay is a
<label>connected to the input as is done in plain HTML.Both of these button group types are established in the new EuiButtonGroupButton component since they share more props/similarities than differences so it's easier to maintain.
3. Additional required props
In addition to changing the actual buttons that are rendered, a few props are now necessary for the accessibility features to work properly.
legend: Required to label the whole group with a single heading. Will be visibly hidden.name: Optional forsingleselections. Applies to thenameattribute of the inputs to keep them grouped. This cannot be the same aslegendbecause of naming collisions. Will generate an string by default.idSelected: Required forsingleselections because radio groups must have an option always selected.Other (quick) changes
i. Added
minWidthprop to EuiButtonThere have been consumer situations where the min-width is not ideal and so this new prop allows consumers to quickly override this CSS property with an inline style.

ii. Added
baseClassNameto EuiButtonDisplayBefore, EuiButtonDisplay was only used by EuiButton which made it ok to hard-code the
euiButtonclass, but now it's being used directly by EuiButtonGroupButton which wanted to provide it's own base class that all the variations are built on.EuiButton still produces all the same class lists it did before, but now EuiButtonDisplay is just more reusable (but still for internal use only).
iii. Updated EuiButton disabled styles to be based on a class and not
:disabledBecause EuiButton can now render as something other than a
<button>, we want to ensure that the disabled styles are applied, so the mixins now are based on.euiButton-isDisabledinstead of:disabled.Checklist