[EuiIcon, EuiButtonIcon] Improved contrast for named colors #4049
Conversation
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4049/ |
elizabetdev
changed the title
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4049/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4049/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4049/ |
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4049/ |
src/components/health/health.tsx
Outdated
| const classes = classNames('euiHealth', className); | ||
|
|
||
| return ( | ||
| <div className={classes} {...rest}> | ||
| <EuiFlexGroup gutterSize="xs" alignItems="center" responsive={false}> | ||
| <EuiFlexItem grow={false}> | ||
| <EuiIcon type="dot" color={color} /> | ||
| <EuiIcon type="dot" {...healthColor} /> |
There was a problem hiding this comment.
Just being nitpicky here but also want to be sure we're following our own conventions. Your classnames you've constructed that would be added here are using the BEM syntax that would be applied to the outer most element euiHealth--primary, but you're applying it to the icon which would normally be called euiHealth__icon--primary.
| success: makeGraphicContrastColor($euiColorSuccess), | ||
| subdued: $euiTextSubduedColor, | ||
| text: $euiTextColor, | ||
| warning: makeGraphicContrastColor($euiColorWarning), |
There was a problem hiding this comment.
Are these all the same values used in EuiHealth? If so, I think it's ok to let the EuiIcon color handle the coloring without actually needing to re-apply the coloring in EuiHealth specifically.
Meaning passing color="accent" from EuiHealth to the embedded EuiIcon will produce the same rendered color (makeHighContrast), so why not just complicate it less.
Though there can be the argument that also having the custom coloring in EuiHealth helps ensure EuiHealth can maintain it's own coloring.
There was a problem hiding this comment.
I reverted the EuiHealth code to what it was originally. This way the EuiIcon is handling the color for it.
This was actually how I started the PR by just changing the EuiIcon colors so I believe it's the best way.
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4049/ |
elizabetdev
changed the title
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_4049/ |
Summary
Closes #3728.
The EuiHealth named colors (primary, secondary, success, accent, warning...) were looking too dark because they were using the SASS function
makeHighContrastColor()that works best for text-specific cases.This PR replaces the named colors that were using
makeHighContrastColor()and replaces with amakeGraphicContrastColor()function. This way, named colors like warning looks lighter.Checklist
[ ] Checked in mobile[ ] Props have proper autodocs[ ] Added documentation[ ] Checked Code Sandbox works for the any docs examples[ ] Added or updated jest tests[ ] Checked for breaking changes and labeled appropriately