-
Notifications
You must be signed in to change notification settings - Fork 837
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
[Emotion] Convert EuiBadge #6224
Conversation
+ 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
Preview documentation changes for this PR: https://eui.elastic.co/pr_6224/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/components/badge/badge.styles.ts
Outdated
disabled: css` | ||
// Using !important to override inline styles | ||
color: ${makeDisabledContrastColor(euiTheme.colors.disabledText)( | ||
euiTheme.colors.disabled | ||
)} !important; | ||
background-color: ${euiTheme.colors.disabled} !important; | ||
`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Love it! 8d572eb
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/components/badge/badge.tsx
Outdated
}; | ||
|
||
let textColor = null; | ||
const wcagContrastBase = 4.5; // WCAG AA contrast level |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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").
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, JS variable. :)
I spend too much time thinking pure CSS than JS for styling.
Preview documentation changes for this PR: https://eui.elastic.co/pr_6224/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 🎉
Preview documentation changes for this PR: https://eui.elastic.co/pr_6224/ |
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:
flex-direction: row-reverse
for a11y (closes [EuiBadge] Incorrect tab order when a clickable badge has a clickable icon on the left side #5353)euiTheme
instead.euiBadge--hollow
,.euiBadge-isClickable
,.euiBadge-isDisabled
,.euiBadge--iconLeft
, and.euiBadge--iconRight
Checklist
- [ ] Checked in mobile- [ ] Added documentation- [ ] Checked Code Sandbox works for any docs examples- [ ] Checked for breaking changes and labeled appropriately- [ ] Updated the Figma library counterpartEmotion checklist
-inline
and-block
(Logical properties).js
files be converted to TS?- [ ] Convert component-specific Sass vars to exported JS versions- [ ] Usegap
property to add margin between items if using flex- [ ] All animations or transitions are wrapped ineuiCanAnimate
QA