Skip to content
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

Merged
merged 10 commits into from
Nov 17, 2020
Merged

Amsterdam: better code block contrast #4272

merged 10 commits into from
Nov 17, 2020

Conversation

snide
Copy link
Contributor

@snide snide commented Nov 16, 2020

Summary

Improves the contrast of code blocks in EUI's Amsterdam theme:

  • Changed the inline code color to our brand pink. Lots of trial and error here. It felt like it needed to be something that wouldn't be confused for a button / link. Purple felt like it stood out without being overpowering. The browns were an OK alternative.
  • Increased the font-weight of code to 500, from 400. Because code already comes in at smaller 90% sizing, the extra weight makes them stand out a bit here.

Why

It was pretty apparent in the docs project that the inline code bits were hard to see.

Before

image

After

image

Darkmode

image

Alternatives considered

image

image

Checklist

  • Check against all themes for compatibility in both light and dark modes
  • Checked in mobile
  • Checked in Chrome, Safari, Edge, and Firefox
  • 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
  • Checked for accessibility including keyboard-only and screenreader modes
  • A changelog entry exists and is marked appropriately

Comment on lines +56 to +57
$euiCodeFontWeightRegular: 400 !default;
$euiCodeFontWeightBold: 700 !default;
Copy link
Contributor Author

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.

@snide snide requested a review from cchaos November 16, 2020 20:57
@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Nov 16, 2020

I really wish there was some weight in between these two (there is if could go variable). The 500 feels way to hefty to me. And I think this is attributed to the different OS's. I also faked using the 700 on a particular selector and now there is no discernable difference. Here's an example of changing that purple selector to bold.

image

Perhaps we leave it as it were in EuiCodeBlock, but the inline version gets bold (700)?

@snide
Copy link
Contributor Author

snide commented Nov 16, 2020

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.

@snide
Copy link
Contributor Author

snide commented Nov 16, 2020

@cchaos 500 for inline, 400 for blocks, which then still allows the 700 difference for bold.

v7 theme still remains unchanged.

image

@snide
Copy link
Contributor Author

snide commented Nov 16, 2020

Only negative to this approach is that we add the extra weight, but meh... seems worth it.

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Nov 17, 2020

I think you meant 500 here (for inline), which I think is fine.

Yeah, no I did actually mean 700 for the same reason as you mentioned earlier of not adding another font weight to the stack.

@snide
Copy link
Contributor Author

snide commented Nov 17, 2020

@cchaos updated. It's a little chunky, but I'll live with it 😈

image

@kibanamachine
Copy link

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

@cchaos
Copy link
Contributor

cchaos commented Nov 17, 2020

Jenkins, test this

@kibanamachine
Copy link

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

Copy link
Contributor

@cchaos cchaos left a 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.

CHANGELOG.md Outdated Show resolved Hide resolved
src-docs/src/theme_amsterdam_dark.scss Outdated Show resolved Hide resolved
@@ -0,0 +1,8 @@
.euiCodeBlock.euiCodeBlock--inline {
border-radius: $euiBorderRadiusSmall;
color: makeHighContrastColor($euiColorVis3, $euiCodeBlockBackgroundColor);
Copy link
Contributor

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?

Suggested change
color: makeHighContrastColor($euiColorVis3, $euiCodeBlockBackgroundColor);
color: $euiCodeBlockKeywordColor;

Copy link
Contributor Author

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.

@kibanamachine
Copy link

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

Co-authored-by: Caroline Horn <549577+cchaos@users.noreply.github.com>
@snide snide merged commit 934b792 into elastic:master Nov 17, 2020
@snide snide deleted the ams/code branch November 17, 2020 19:02
@kibanamachine
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants