Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Deprecating EuiToggle and EuiToggleButton #3099

Closed
wants to merge 17 commits into from
Closed

Deprecating EuiToggle and EuiToggleButton #3099

wants to merge 17 commits into from

Conversation

myasonik
Copy link
Contributor

@myasonik myasonik commented Mar 17, 2020 •
edited
Loading

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

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

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Firefox
  • Checked in IE11
  • Props have proper autodocs
  • Added documentation examples
  • Added or updated jest tests
  • Checked for breaking changes and labeled appropriately
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

@myasonik myasonik added breaking change accessibility deprecations Contains deprecations. Add them to the deprecations meta ticket after merge. labels Mar 17, 2020
@kibanamachine

This comment has been minimized.

Sign in to view
cchaos
cchaos reviewed Mar 17, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
src-docs/src/views/button/button_example.js Outdated Show resolved Hide resolved
src/components/button/button_icon/button_icon.tsx Outdated Show resolved Hide resolved
src/components/button/button_group/button_group.tsx Outdated Show resolved Hide resolved
src/components/button/button_group/button_group.tsx Outdated Show resolved Hide resolved
@kibanamachine

This comment has been minimized.

Sign in to view
@cchaos

This comment has been minimized.

Sign in to view
@myasonik

This comment has been minimized.

Sign in to view
@cchaos

This comment has been minimized.

Sign in to view
@myasonik

This comment has been minimized.

Sign in to view
@kibanamachine

This comment has been minimized.

Sign in to view
@cchaos

This comment has been minimized.

Sign in to view
@kibanamachine

This comment has been minimized.

Sign in to view
@cchaos

This comment has been minimized.

Sign in to view
@myasonik

This comment has been minimized.

Sign in to view
@kibanamachine

This comment has been minimized.

Sign in to view
@myasonik

This comment has been minimized.

Sign in to view
@kibanamachine

This comment has been minimized.

Sign in to view
cchaos
cchaos reviewed Apr 9, 2020
View reviewed changes
Copy link
Contributor

@cchaos cchaos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/components/button/button.tsx Show resolved Hide resolved
src/components/button/button_icon/button_icon.tsx Outdated
Comment on lines 32 to 35
export type HideOrLabel = ExclusiveUnion<
{ 'aria-hidden': true },
ExclusiveUnion<{ 'aria-label': string }, { 'aria-labelledby': string }>
>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"

Screen Shot 2020-04-09 at 13 50 37 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦 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 Outdated Show resolved Hide resolved
src/components/date_picker/super_date_picker/quick_select_popover/quick_select.js Outdated Show resolved Hide resolved
@cchaos

This comment has been minimized.

Sign in to view
@kibanamachine
Copy link

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

@myasonik
Copy link
Contributor Author

myasonik commented Jun 11, 2020 •
edited
Loading

@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
Copy link

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

@chandlerprall
Copy link
Contributor

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.

group options props

@myasonik
Copy link
Contributor Author

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!

@myasonik myasonik requested a review from cchaos June 15, 2020 23:26
@myasonik myasonik marked this pull request as ready for review June 15, 2020 23:26
@kibanamachine
Copy link

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

@kibanamachine
Copy link

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

cchaos
cchaos reviewed Jun 16, 2020
View reviewed changes
src/components/button/button_group/button_group_single.tsx
Comment on lines +59 to +63
'euiButton--disabled': isGroupDisabled || isDisabled,
'euiButtonGroup__toggle--iconOnly': isIconOnly,
'euiButton--fill': fill,
'euiButton--small': size === 'compressed' || size === 's',
'euiButton--ghost': size !== 'compressed' && color === 'ghost',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@cchaos cchaos mentioned this pull request Jul 13, 2020
Merged
3 tasks
@cchaos cchaos mentioned this pull request Sep 4, 2020
Closed
@cchaos cchaos mentioned this pull request Sep 19, 2020
Merged
10 tasks
@cchaos cchaos marked this pull request as draft October 12, 2020 19:37
@myasonik
Copy link
Contributor Author

Closed in favor of #4056

@myasonik myasonik closed this Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cchaos cchaos cchaos left review comments

Assignees
No one assigned
Labels
accessibility breaking change deprecations Contains deprecations. Add them to the deprecations meta ticket after merge.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@myasonik @kibanamachine @cchaos @chandlerprall @thompsongl