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

Badge allow these two events iconOnClick and onClick to trigger #1994

Merged
merged 10 commits into from
Jun 5, 2019

Conversation

XavierM
Copy link
Contributor

@XavierM XavierM commented Jun 4, 2019

Summary

First of all, thanks to @cchaos for helping me and move this forward.

On Firefox and IE11 browser, when these two properties onClick and iconOnClick are provided on the Badge component, you will only get the event onClick working. This PR is fixing that for SIEM and Discover

image

Checklist

  • This was checked in mobile
  • This was checked in IE11
  • This was checked in dark mode
  • Any props added have proper autodocs
  • Documentation examples were added
  • A changelog entry exists and is marked appropriately
  • This was checked for breaking changes and labeled appropriately
  • Jest tests were updated or added to match the most common scenarios
  • This was checked against keyboard-only and screenreader scenarios
  • This required updates to Framer X components

@XavierM XavierM self-assigned this Jun 4, 2019
@XavierM XavierM added the bug label Jun 4, 2019
@cchaos
Copy link
Contributor

cchaos commented Jun 4, 2019

There were two problems happening in EuiBadge if provided both onClick and iconOnClick.

  1. FF and IE don't bubble up an onClick inside of an onClick
  2. The iconOnClick was adding an onClick directly to EuiIcon and therefore an <svg>

Since we can't just wrap the icon with a <button> tag and call it a day since that would mean a button inside of a button, we needed an edge case to be provided where if both onClick and iconOnClick were passed, that the buttons would be side by side therefore it made sense to turn the <span> that wraps the children that sit next to the icon into a <button> for this particular case.

@XavierM
Copy link
Contributor Author

XavierM commented Jun 4, 2019

Would it be possible to backport this in 7.2?

src/components/badge/badge.test.js Outdated Show resolved Hide resolved
src/components/badge/badge.js Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src-docs/src/views/badge/badge_button.js Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor

cchaos commented Jun 4, 2019

It looks like there's some style work to do since wrapping the children in a <button> tag resets all styles:

Screen Shot 2019-06-04 at 18 35 02 PM

@cchaos
Copy link
Contributor

cchaos commented Jun 4, 2019

@XavierM I have pushed a commit directly to your branch to fix the styles. It now looks correct:

Screen Shot 2019-06-04 at 19 02 51 PM

yarn.lock Show resolved Hide resolved
@XavierM XavierM requested a review from cchaos June 5, 2019 00:38
@XavierM XavierM changed the title Badge allow these two events iconOnClick and onClick working together Badge allow these two events iconOnClick and onClick to trigger Jun 5, 2019
@cchaos
Copy link
Contributor

cchaos commented Jun 5, 2019

@XavierM I've pushed one more commit that just adding this callout in the documentation

Screen Shot 2019-06-05 at 09 50 18 AM

You'll also need to create a changelog entry.

@cchaos cchaos requested a review from thompsongl June 5, 2019 14:20
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

code changes so far look fine, but keyboard navigation experience has been degraded:

Previously, tabbing to the icon highlights it
highlighted icon

Now it does a weird box/outline that is barely visible (middle icon is focused)
outlined icon

When both onClick and onIconClick are present the badge does not receive an outline when focused, and the badge text's tab order comes after the icon - first tab selects icon & triggers onIconClick, second tab selects the badge context & triggers onClick

@cchaos
Copy link
Contributor

cchaos commented Jun 5, 2019

Ahh I see the icon focus state styles now. Pushed a fix for that. I'm wasn't really sure how to deal with the button inside the badge focus state so I'm using :focus-within so any time any button (including the icon) is focused it will get the focus ring:

@snide / @ryankeairns Any thoughts on that?

CHANGELOG.md Outdated Show resolved Hide resolved
@cchaos
Copy link
Contributor

cchaos commented Jun 5, 2019

Ugh, it looks like :focus-within is not only not supported by IE, but it completely wipes the normal :focus state.

@thompsongl
Copy link
Contributor

the badge text's tab order comes after the icon - first tab selects icon & triggers onIconClick, second tab selects the badge context & triggers onClick

This is still true with the latest changes. Seems like it should be reversed

@cchaos
Copy link
Contributor

cchaos commented Jun 5, 2019

The latest in the great :focus epic:

Pretty hideous... Not sure what else to do...

@cchaos
Copy link
Contributor

cchaos commented Jun 5, 2019

Alright final round with hover and focus states:

I left one :focus-within in for those that support it. In IE, it'll still show the focus ring on the normal Badge is a Button style, but it just won't show it in the other types. Which is really ok, since there are other states to indicate focus.

@cchaos cchaos requested a review from snide June 5, 2019 16:40
Copy link
Contributor

@chandlerprall chandlerprall left a comment

Choose a reason for hiding this comment

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

Changes LGTM; pulled & tested locally

@cchaos cchaos merged commit bd7778b into elastic:master Jun 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants