Deprecating EuiToggle and EuiToggleButton by myasonik · Pull Request #3099 · elastic/eui

myasonik · 2020-03-17T02:09:42Z

Summary

Deprecates EuiToggle and EuiToggleButton closing #3023

EuiToggle and EuiToggleButton weren't very accessible. To improve accessibility of EUI, we should deprecate them and migrate to instead using aria-pressed on EuiButton and EuiButtonIcon as appropriate.

Breaking changes

EuiButtonGroup now requires legend (previously optional) and type (previously defaulted to 'single' but not having a default lets us have way better types)
Removed optional name prop from EuiButtonGroupProps which was unused
Replaced EuiButtonGroupOptionProps with EuiButtonMultiGroupOptionProps and EuiButtonSingleGroupOptionProps (and moved some prop requirements from code to types)
Moved EuiButtonIcon aria-label/aria-lablledby prop requirements from code to types

Checklist

cchaos

I'll come in with style commit for you but here are some of my initial thoughts.

src-docs/src/views/button/button_example.js

src/components/button/button_icon/button_icon.tsx

src/components/button/button_group/button_group.tsx

cchaos

I'm working through the docs and styles but had some questions about some changes.

src/components/button/button.tsx

cchaos · 2020-04-09T17:59:34Z

src/components/button/button_icon/button_icon.tsx

+export type HideOrLabel = ExclusiveUnion<
+  { 'aria-hidden': true },
+  ExclusiveUnion<{ 'aria-label': string }, { 'aria-labelledby': string }>
+>;


Are you sure aria-hidden should be true by default on an icon button? Before, we would, not so quietly, require aria-label or aria-labelledby on EuiButtonIcon. Otherwise the screen reader just reads: "button"

🤔 How are you getting that? When I run through the preview link it works as expected (reading "Next, button").

And the TS definition should* force a dev to pass in one of the three props which maintains the same functionality that this used to have (the console.warn would be suppressed if they passed in aria-hidden. If you're up for it, I'd happily cut the ability to pass in aria-hidden here because you're totally right that it's a terrible experience.

*: I say "should" here because I didn't test it thoroughly

Btw, I got it just from removing the aria-labels from the examples. In this PR, the button now no longer complains about a non-exisiting aria-label. Which it should.

OK, yeah, that was bad. I've tried again.

So aria-hidden is never allowed (we just should not be hiding a button or link from screen readers). And either aria-label or aria-labelled is required.

🤦 Ignore that. EUI had a valid use case inside DataGrid for aria-hidden on a button. So I allow aria-hidden only with tabIndex=-1 and disallow trying to label it as well so people (hopefully) understand it won't work for screen readers or keyboard-only users.

src/components/button/button.tsx

src/components/date_picker/super_date_picker/quick_select_popover/quick_select.js

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

kibanamachine · 2020-06-08T23:09:00Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3099/

myasonik · 2020-06-11T22:55:09Z

@chandlerprall I force pushed latest because some datagrid tests are failing and props for EuiButtonGroup(Multi|Single)OptionProps won't render... 🤨 Maybe you could take a look?

kibanamachine · 2020-06-11T23:13:58Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3099/

chandlerprall · 2020-06-12T16:43:29Z

DataGrid errors

The sort selector previously had the euiButtonGroup__button--selected class, it no longer does (looks like it was removed from type=single). The sort selector has also lost its filled (activated) styles when compressed, which is visually affecting data grid.

EuiButtonGroup*OptionProps

They render for me. This is likely a cache issue where webpack is not made aware of the dependency on downstream type changes (blergh). Try changing the divs to spans in src-docs/src/views/button/props.tsx to refresh the cache, and if that doesn't work stop your server, run rm -rf .cache-loader, and re-start the server.

myasonik · 2020-06-15T23:26:30Z

OK, I tested mobile, Light, Dark, Amsterdam Light, Amsterdam Dark, and ghost.

I probably missed some permutations but I think it's ready for others to look at!

kibanamachine · 2020-06-15T23:47:12Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3099/

kibanamachine · 2020-06-16T17:37:38Z

Preview documentation changes for this PR: https://eui.elastic.co/pr_3099/

cchaos · 2020-06-16T18:29:06Z

src/components/button/button_group/button_group_single.tsx

+      'euiButton--disabled': isGroupDisabled || isDisabled,
+      'euiButtonGroup__toggle--iconOnly': isIconOnly,
+      'euiButton--fill': fill,
+      'euiButton--small': size === 'compressed' || size === 's',
+      'euiButton--ghost': size !== 'compressed' && color === 'ghost',


This is a big anti-pattern I'd really like to avoid and what SASS mixins are for. It's going to take me a while, but I'll fix it.

myasonik · 2020-10-16T17:23:19Z

Closed in favor of #4056

deprecating EuiToggle and EuiToggleButton

dbfeff9

myasonik added breaking change PRs with breaking changes. (Don't delete - used for automation) accessibility deprecations PRs that contains deprecations. Add them to the deprecations meta ticket after merge. labels Mar 17, 2020

This comment has been minimized.

Sign in to view

cchaos reviewed Mar 17, 2020

View reviewed changes

Michail Yasonik and others added 4 commits March 17, 2020 12:44

wip cleaning up ButtonIcon types

ccecc4b

untanglaing ToggleButton from ButtonGroups; making the styles so bad

6b2422b

fixing ControlBar types

963da2b

fixing tests

8402c4e

This comment has been minimized.

Sign in to view

myasonik force-pushed the deprecate-toggle branch from 7aa7ea5 to 65fef52 Compare March 31, 2020 00:54

undoing euiButtonIcon change and fixing euiButtonGroup styling

94c78b6

This comment has been minimized.

Sign in to view

fixing more style things

918bebd

myasonik force-pushed the deprecate-toggle branch from 65fef52 to 918bebd Compare April 2, 2020 00:47

This comment has been minimized.

Sign in to view

Merge remote-tracking branch 'upstream/master' into deprecate-toggle

56390b9

This comment has been minimized.

Sign in to view

Michail Yasonik added 2 commits April 3, 2020 22:22

one step forward, one step back

b647d46

Merge remote-tracking branch 'upstream/master' into deprecate-toggle

b64761f

This comment has been minimized.

Sign in to view

light theme seems to be good

d5399f9

This comment has been minimized.

Sign in to view

cchaos reviewed Apr 9, 2020

View reviewed changes

This comment has been minimized.

Sign in to view

Michail Yasonik and others added 3 commits June 8, 2020 17:42

Merge remote-tracking branch 'upstream/master' into deprecate-toggle

41475b6

merge conflict leftovers

b8a58ec

incorporating feedback from @cchaos

24f1b61

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>

types and componts refactored

66d4024

cleanup

cc03131

myasonik requested a review from cchaos June 15, 2020 23:26

myasonik marked this pull request as ready for review June 15, 2020 23:26

Merge branch 'master' into deprecate-toggle

d60625e

cchaos reviewed Jun 16, 2020

View reviewed changes

cchaos mentioned this pull request Jul 13, 2020

Chore/decouple button content #3730

Merged

3 tasks

cchaos mentioned this pull request Sep 4, 2020

[EuiKeyPadMenu] Add selection state #4000

Closed

cchaos mentioned this pull request Sep 19, 2020

Fix EuiButtonGroup a11y and remove EuiToggle/EuiButtonToggle #4056

Merged

10 tasks

cchaos marked this pull request as draft October 12, 2020 19:37

myasonik closed this Oct 16, 2020

Conversation

myasonik commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Breaking changes

Checklist

Uh oh!

This comment has been minimized.

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

cchaos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

cchaos Apr 9, 2020

Choose a reason for hiding this comment

Uh oh!

myasonik Apr 10, 2020

Choose a reason for hiding this comment

Uh oh!

cchaos Apr 15, 2020

Choose a reason for hiding this comment

Uh oh!

myasonik Jun 11, 2020

Choose a reason for hiding this comment

Uh oh!

myasonik Jun 15, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

kibanamachine commented Jun 8, 2020

Uh oh!

myasonik commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kibanamachine commented Jun 11, 2020

Uh oh!

chandlerprall commented Jun 12, 2020

DataGrid errors

EuiButtonGroup*OptionProps

Uh oh!

myasonik commented Jun 15, 2020

Uh oh!

kibanamachine commented Jun 15, 2020

Uh oh!

kibanamachine commented Jun 16, 2020

Uh oh!

cchaos Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

myasonik commented Oct 16, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

myasonik commented Mar 17, 2020 •

edited

Loading

myasonik commented Jun 11, 2020 •

edited

Loading