Skip to content

Remove EuiKeyboardAccessible#4991

Merged
myasonik merged 4 commits intoelastic:masterfrom
myasonik:mostly-remove-euikeyboardaccessible
Jul 30, 2021
Merged

Remove EuiKeyboardAccessible#4991
myasonik merged 4 commits intoelastic:masterfrom
myasonik:mostly-remove-euikeyboardaccessible

Conversation

@myasonik
Copy link
Copy Markdown
Contributor

@myasonik myasonik commented Jul 28, 2021

Summary

EuiKeyboardAccessible has been deprecated since Oct 2020 and all instances have been removed from Kibana. Removed it from EUI though rewrote a small version of it to fill in the gap left in EuiBasicTable which uses it for clickable rows. (That's it's own a11y issue but a difficult and more involved fix so not going to tackle it all at once. #4155)

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • Props have proper autodocs and playground toggles
  • Added documentation
  • Checked Code Sandbox works for the any docs 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 the breaking change PRs with breaking changes. (Don't delete - used for automation) label Jul 28, 2021
@myasonik myasonik force-pushed the mostly-remove-euikeyboardaccessible branch from 76eacf7 to c3a5a36 Compare July 29, 2021 03:50
@kibanamachine
Copy link
Copy Markdown

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


// TODO remove - this implementation is not actually accessible
// https://github.com/elastic/eui/issues/4155
export class EuiKeyboardAccessible extends Component<Props> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we at least re-name to be table specific? So that when we "get rid of it", it might actually just be implementing a better solution to making the rows clickable.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So... I ended up removing it afterall and reimplementing the same solution inline in EuiTableRow (but with only the bits it needs).

Assuming this merges, I'll update #4155 with the latest details.

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

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

@thompsongl thompsongl self-requested a review July 29, 2021 17:16
@kibanamachine
Copy link
Copy Markdown

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

Copy link
Copy Markdown
Contributor

@thompsongl thompsongl left a comment

Choose a reason for hiding this comment

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

LGTM 🙏
Maybe update the summary to reflect a full removal

@myasonik myasonik changed the title Mostly remove EuiKeyboardAccessible Remove EuiKeyboardAccessible Jul 30, 2021
@myasonik myasonik requested a review from cchaos July 30, 2021 17:20
@kibanamachine
Copy link
Copy Markdown

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

Copy link
Copy Markdown
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.

🙇 Thank you for moving that into the table row.

I checked that the rows were at least still focusable but I couldn't find any existing examples that do anything with the event. So LGTM 🤷 😆

@myasonik myasonik merged commit 8e51c65 into elastic:master Jul 30, 2021
@myasonik myasonik deleted the mostly-remove-euikeyboardaccessible branch July 30, 2021 17:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking change PRs with breaking changes. (Don't delete - used for automation)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants