-
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
Amsterdam: better code block contrast #4272
Conversation
$euiCodeFontWeightRegular: 400 !default; | ||
$euiCodeFontWeightBold: 700 !default; |
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.
Added these to mainline as well without changing their values. Felt like we needed the separation.
Preview documentation changes for this PR: https://eui.elastic.co/pr_4272/ |
I really wish there was some weight in between these two (there is if could go variable). The Perhaps we leave it as it were in EuiCodeBlock, but the inline version gets bold (700)? |
@cchaos I think you meant 500 here (for inline), which I think is fine. I actually noticed a bug with the inter css I was importing in the docs repo that seemed to make overly light weights in roboto somehow. Changing it to the all google fonts method you're using fixes the issues I was seeing. I still think the purple helps as well (for inline), so I'm going to keep that. |
@cchaos v7 theme still remains unchanged. |
Only negative to this approach is that we add the extra weight, but meh... seems worth it. |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4272/ |
Yeah, no I did actually mean 700 for the same reason as you mentioned earlier of not adding another font weight to the stack. |
@cchaos updated. It's a little chunky, but I'll live with it 😈 |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4272/ |
Jenkins, test this |
Preview documentation changes for this PR: https://eui.elastic.co/pr_4272/ |
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 agree it's hefty, but I think we can go with this for now and since it only affects Amsterdam, we can decide if we really want to add that third weight.
@@ -0,0 +1,8 @@ | |||
.euiCodeBlock.euiCodeBlock--inline { | |||
border-radius: $euiBorderRadiusSmall; | |||
color: makeHighContrastColor($euiColorVis3, $euiCodeBlockBackgroundColor); |
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.
Maybe use a code variable that is the same?
color: makeHighContrastColor($euiColorVis3, $euiCodeBlockBackgroundColor); | |
color: $euiCodeBlockKeywordColor; |
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.
Gonna leave this. I prefer using the root variable until there is more than one usage that exists.
Preview documentation changes for this PR: https://eui.elastic.co/pr_4272/ |
Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
Preview documentation changes for this PR: https://eui.elastic.co/pr_4272/ |
Summary
Improves the contrast of code blocks in EUI's Amsterdam theme:
Why
It was pretty apparent in the docs project that the inline code bits were hard to see.
Before
After
Darkmode
Alternatives considered
Checklist