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

[EuiKeyPadMenuItem] Moves beta badge tooltip props to a wrapping EuiToolTip #5541

Merged
merged 5 commits into from
Jan 18, 2022

Conversation

cchaos
Copy link
Contributor

@cchaos cchaos commented Jan 14, 2022

Fixes #5290 based on explorations in #5508

@1Copenut, I was reviewing your latest changes in #5508, and had a thought about this whole ordeal. I wanted to go back to the original though around why we needed/wanted "Beta badges". Essentially they give further description to the button by providing a small visual indicator with fulll description through a tooltip.

The original implementation relied solely on the EuiBetaBadge's own tooltip to provide that functionality. This was a mistake. Not just for screen-reader accessibility but also for touch points and discoverability. Essentially only allowing a small 24x24 area to be the trigger for this tooltip, even though it's giving more context to the whole button.

Also no matter what we did to correctly pull out this badge from the button and hook up the descriptions through aria- props, we ended up with a disjointed UI. Meaning, each button had two tabs to get to the next, and once the user's focus is on the beta badge, the context of the button's label is lost.

So... going back to the intent of the beta badge, is to show a (1.) visual indicator and (2.) tooltip with more information.

Since we ask for the beta badge props individually, we can pull out the specific tooltip ones and pass that to a wrapping EuiTooltip instead. Thus removes any interactivity from the beta badge itself, just becoming something purely visual (1.) and puts the tooltip on the already interactive element that the key pad menu renders (2.).

This is what renders:

Screen Shot 2022-01-14 at 15 43 38 PM


🔔 Breaking

The breaking change here would only be that, when a beta badge exists, consumers who need to customize the outer element need to use the new betaBadgeTooltipProps to pass a custom anchorClassName too. This is more based on how EuiToolTip works which wraps the child in this anchor element.


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 any docs examples
  • Added or updated jest and cypress 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

@kibanamachine
Copy link

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

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

This is a much improved solution over the exploration I was doing. This reorganizes the beta badge as a visual cue that doesn't diminish the experience for keyboard navigation or screen reader use. This solution also passes the axe browser check for not having nested focusable elements.

@1Copenut
Copy link
Contributor

@cchaos I think this is a stronger approach than my original PR #5508. I'll keep that one open until this gets approved and merged.

@cchaos cchaos changed the title [WIP] [EuiKeyPadMenuItem] Moves beta badge tooltip props to a wrapping EuiToolTip [EuiKeyPadMenuItem] Moves beta badge tooltip props to a wrapping EuiToolTip Jan 18, 2022
@cchaos cchaos requested a review from 1Copenut January 18, 2022 21:44
@kibanamachine
Copy link

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

Copy link
Contributor

@1Copenut 1Copenut left a comment

Choose a reason for hiding this comment

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

👍 LGTM! Thank you for adding the aria-hidden attribute to the beta badge.

@cchaos cchaos marked this pull request as ready for review January 18, 2022 22:32
@kibanamachine
Copy link

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

@cchaos cchaos enabled auto-merge (squash) January 18, 2022 23:09
@cchaos cchaos merged commit 4960592 into elastic:main Jan 18, 2022
@cchaos cchaos deleted the keypad_beta branch January 19, 2022 14:36
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.

[EuiKeyPadMenu][AXE-CORE]: Nested elements that can take focus will not be announced by screen readers
3 participants