-
Notifications
You must be signed in to change notification settings - Fork 841
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
Increase contrast for EuiCodeBlock colors #3309
Increase contrast for EuiCodeBlock colors #3309
Conversation
Preview documentation changes for this PR: https://eui.elastic.co/pr_3309/ |
Need to merge master... |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3309/ |
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.
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_3309/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3309/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3309/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3309/ |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3309/ |
@cchaos and @ryankeairns, Here are a few design decisions that I took to complete this PR:
|
Preview documentation changes for this PR: https://eui.elastic.co/pr_3309/ |
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 love it! This looks soooooo much better. Small comment about a couple of your additional variable names. Also for the addition / deletion stuff there's no rendering in the actual docs, but I'm guessing that's what you meant by making a separate issue for it?
$euiCodeBlockPurple: #6F42C1; | ||
$euiCodeBlockOrange: #E36209; |
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 would name these not by their color but by their purpose.
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.
Ugh. Github ate my full comment. This one is tricky. On one hand these are being used how we'd normally use a core value like $euiPrimary
, or they are just one offs for use in the code blocks alone. It's hard to make a name for these things without writing the descriptive name like euiCodeBlockKeywordColor
over again. Whichever direction we go, I think it's safe to say it should be the name of the color. Even something like $euiColorCodeBlock01
and $euiColorCodeBlock02
feels more flexible.
I'm sure @cchaos has opinions.
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.
Related yet tangental thought to this. Maybe we should be using the color blind palette instead of our primary, secondary, etc... colors which are more specific to our brand whereas the code block coloring is more specific to visually distinguishing between elements of code. We even have purple and orange colors in there where we then won't have to redefine new hexes for those.
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.
Here are the new updates:
- As suggested, I replaced the colors with the color blind palette.
- The colors
$euiTextSubduedColor
and$euiTextColor
were not replaced because the color blind palette doesn't have colors for texts. - I didn't replace the $euiColorDanger because we don't have a red in the color blind palette.
- I checked the contrast levels and all of them passed 4.5.
Let me know what you think of this new version:
Thanks @miukimiu , the color contrast levels check out and it looks much better! |
Preview documentation changes for this PR: https://eui.elastic.co/pr_3309/ |
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.
It is too bad that we do loose some saturation in the colors, but I think it's really only noticeable when doing a directly comparison. In the long run, it's a much better correlation to use the vis colors instead of the brand colors.
👍 LGTM!
Fixes #3243
Summary
The color variables used by
EuiCodeBlock
produce insufficient color contrast largely due to the slightly darker background color ($euiColorLightestShade
vs$euiPageColorBackground
).This PR:
makeHighContrastColor
mixin as prescribed in the underlying issue.Testing
It's a little cumbersome to test all the color variables. You can do a couple of things to render the less used colors:
hljs-meta
,hljs-title
,hljs-section
,hljs-addition
,hljs-deletion
, etc.) See the_code_block.scss
file for the full set; many of these render in the existing examples already.c++
example from the hljs docs to render several of the aforementioned classes:Renders as... (and all passed 4.5):
Checklist
Checked in mobileProps have proper autodocsAdded or updated jest tests