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.

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

Remove EuiKeyboardAccessible #4991

Merged
merged 4 commits into from
Jul 30, 2021

Conversation

myasonik
Copy link
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 force-pushed the mostly-remove-euikeyboardaccessible branch from 76eacf7 to c3a5a36 Compare July 29, 2021 03:50
@kibanamachine
Copy link

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

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -39,6 +39,8 @@ interface Props {
children: ReactElement;
}

// TODO remove - this implementation is not actually accessible
// https://github.com/elastic/eui/issues/4155
export class EuiKeyboardAccessible extends Component<Props> {
Copy link
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
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 <[email protected]>
@kibanamachine
Copy link

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

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

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

@thompsongl thompsongl mentioned this pull request Jul 30, 2021
35 tasks
@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

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

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.

🙇 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants