[Emotion] Convert EuiBadge #6224

cee-chen · 2022-09-08T22:28:17Z

Summary

Note that this PR only handles EuiBadge and not EuiBadgeGroup, or EuiBetaBadge, or EuiNotificationBadge. I thought this PR was getting long enough as it and preferred an easier to review and more atomic PR. I also strongly recommend following along by commit.

In addition to 1:1 Sass->Emotion conversions, this PR also:

Fixes clickable buttons without clickable icons having the wrong cursor (default instead of pointer)
Fixes icon position to use source order instead of flex-direction: row-reverse for a11y (closes [EuiBadge] Incorrect tab order when a clickable badge has a clickable icon on the left side #5353)
Addresses various TODOs around static color strings by using euiTheme instead
Removes the following modifier classes: .euiBadge--hollow, .euiBadge-isClickable, .euiBadge-isDisabled, .euiBadge--iconLeft, and .euiBadge--iconRight

Checklist

Checked in both light and dark modes
Checked in Chrome, Safari, Edge, and Firefox
Props have proper autodocs and playground toggles
Added or updated jest and cypress tests
Checked for accessibility including keyboard-only and screenreader modes
A changelog entry exists and is marked appropriately

- [ ] Checked in mobile
- [ ] Added documentation
~~- [ ] Checked Code Sandbox works for any docs examples~~
~~- [ ] Checked for breaking changes and labeled appropriately~~
~~- [ ] Updated the Figma library counterpart~~

Emotion checklist

Anything in the backlog that could be a quick fix for that component?
Convert global Sass vars to JS version like sizing
Convert side specific padding, margin, and position to -inline and -block (Logical properties)
Reduce specificity where possible (usually by reducing class name chaining)
Can any still existing .js files be converted to TS?

~~- [ ] Convert component-specific Sass vars to exported JS versions~~
~~- [ ] Use gap property to add margin between items if using flex~~
~~- [ ] All animations or transitions are wrapped in euiCanAnimate~~

QA

Does the output CSS match the previous CSS / as expected
Does the rendered class read as expected

+ fix unnecessary nesting / remove need for `flex-direction: row-reverse` by using optional JSX instead - note that removing `row-reverse` also accomplishes several things: accessibility/focus order will now match tab order, and `direction: rtl` correctly flips the icon position

+ remove need for nested selector + fix cursor not being an actual pointer on badges with only onClick

- removing nesting and margin unsetting in favor of Emotion array logic

- by passing/using euiTheme - clarify badge color options - not that it super matters as it basically comes down to `string` in any case + memoize optionalCustomStyles / color logic w/ some simplifications

kibanamachine · 2022-09-08T22:59:57Z

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

miukimiu

I looked at the code and tested it in Chrome, Safari, Firefox, and Edge. LGTM! 🎉

I also looked at the EUI Figma library and found that there were some updates to the disabled badges that were never implemented. So I guess this is a good opportunity to make those updates.

miukimiu · 2022-09-13T15:45:05Z

src/components/badge/badge.styles.ts

+    disabled: css`
+      // Using !important to override inline styles
+      color: ${makeDisabledContrastColor(euiTheme.colors.disabledText)(
+        euiTheme.colors.disabled
+      )} !important;
+      background-color: ${euiTheme.colors.disabled} !important;
+    `,


We have a new design for the disabled state in EUI Figma.

So it will look like the last row. More similar to a disabled EuiButton.

I think you can even use the disabled button colors:

euiButtonColor(euiThemeContext, 'disabled').color} euiButtonColor(euiThemeContext, 'disabled').backgroundColor}

Love it! 8d572eb

1Copenut

LGTM! I left one comment about moving the hard-coded value "4.5" in badge.tsx#152 into a custom property. That's a nice to have and shouldn't hold up merging the PR.

1Copenut · 2022-09-13T16:58:53Z

src/components/badge/badge.tsx

+    };
+
+    let textColor = null;
+    const wcagContrastBase = 4.5; // WCAG AA contrast level


I think this would be a good candidate for a CSS custom property. Doesn't need to be today, but this number of the measurement may change with WCAG 3.0 (aka "Silver").

CSS custom property

Do you mean an EUI JS variable? If so, agreed (and it was actually a previous TODO that I can add back in), but TBH I don't 100% know where it should live.

Maybe src/services/color/contrast.ts as a general export?

After looking more closely, I really like src/services/color/contrast for locating this var as a general export and I think it fits there the most. I've made it part of the PR: 617dd30

Yep, JS variable. :)

I spend too much time thinking pure CSS than JS for styling.

kibanamachine · 2022-09-13T17:06:14Z

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

miukimiu

LGTM! 🎉

kibanamachine · 2022-09-13T17:48:58Z

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

cee-chen added 10 commits September 8, 2022 15:16

Convert base .euiBadge className + modifiers

ae233a8

Convert .euiBadge__text CSS

71559ec

+ remove need for nested selector + fix cursor not being an actual pointer on badges with only onClick

Convert .euiBadge__icon and .euiBadge__iconButton CSS

6207432

- removing nesting and margin unsetting in favor of Emotion array logic

Convert .euiBadge__childButton to CSS

1bd5dad

Delete badge.scss

4025069

Remove iconSideToClassNameMap

cb566ad

Address static color string TODOs

b35e8af

- by passing/using euiTheme - clarify badge color options - not that it super matters as it basically comes down to `string` in any case + memoize optionalCustomStyles / color logic w/ some simplifications

Update tests and snapshots

d5bf52a

[misc docs] Convert docs examples from JS to TSX

c2d4740

cee-chen added the emotion label Sep 8, 2022

Changelog

6ea83d6

cee-chen requested a review from 1Copenut September 8, 2022 22:32

cee-chen marked this pull request as ready for review September 8, 2022 22:32

cee-chen requested a review from miukimiu September 12, 2022 19:26

miukimiu reviewed Sep 13, 2022

View reviewed changes

cee-chen added 2 commits September 13, 2022 09:29

Merge branch 'main' into emotion/badge

d7d3177

[PR feedback] Update disabled badge style to use disabled button styles

8d572eb

cee-chen requested a review from miukimiu September 13, 2022 16:36

cee-chen mentioned this pull request Sep 13, 2022

[EuiBadge] Incorrect tab order when a clickable badge has a clickable icon on the left side #5353

Closed

1Copenut approved these changes Sep 13, 2022

View reviewed changes

[PR feedback] Extract WCAG 4.5 ratio const to shared export

617dd30

miukimiu approved these changes Sep 13, 2022

View reviewed changes

cee-chen enabled auto-merge (squash) September 13, 2022 17:26

cee-chen merged commit c9d341d into elastic:main Sep 13, 2022

cee-chen deleted the emotion/badge branch September 13, 2022 18:18

breehall mentioned this pull request Sep 21, 2022

Bumping EUI to version 67.1.8 elastic/kibana#141279

Merged

This was referenced Sep 21, 2022

[Emotion] Convert EuiBadgeGroup, EuiBetaBadge, and EuiNotificationBadge #6258

Merged

[Meta][CSS-in-JS] Component conversions #5685

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Emotion] Convert EuiBadge #6224

[Emotion] Convert EuiBadge #6224

cee-chen commented Sep 8, 2022 •

edited

Loading

kibanamachine commented Sep 8, 2022

miukimiu left a comment

miukimiu Sep 13, 2022

cee-chen Sep 13, 2022

1Copenut left a comment

1Copenut Sep 13, 2022

cee-chen Sep 13, 2022

cee-chen Sep 13, 2022

1Copenut Sep 13, 2022

kibanamachine commented Sep 13, 2022

miukimiu left a comment

kibanamachine commented Sep 13, 2022

[Emotion] Convert EuiBadge #6224

[Emotion] Convert EuiBadge #6224

Conversation

cee-chen commented Sep 8, 2022 • edited Loading

Summary

Checklist

Emotion checklist

QA

kibanamachine commented Sep 8, 2022

miukimiu left a comment

Choose a reason for hiding this comment

miukimiu Sep 13, 2022

Choose a reason for hiding this comment

cee-chen Sep 13, 2022

Choose a reason for hiding this comment

1Copenut left a comment

Choose a reason for hiding this comment

1Copenut Sep 13, 2022

Choose a reason for hiding this comment

cee-chen Sep 13, 2022

Choose a reason for hiding this comment

cee-chen Sep 13, 2022

Choose a reason for hiding this comment

1Copenut Sep 13, 2022

Choose a reason for hiding this comment

kibanamachine commented Sep 13, 2022

miukimiu left a comment

Choose a reason for hiding this comment

kibanamachine commented Sep 13, 2022

cee-chen commented Sep 8, 2022 •

edited

Loading