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

Set up sass functions folder, add new functions for color contrast #628

Merged
merged 6 commits into from
Apr 4, 2018

Conversation

snide
Copy link
Contributor

@snide snide commented Apr 3, 2018

In a very roundabout way fixes #604

What this PR does:

  1. Separates Sass functions into a new /global_styling/functions/ folder so that they don't dirty up the rest of the files. These get imported along with global_styling genetically like our other mixins.
  2. Adds new math and color functions:
    • A bunch of math functions for the single purpose of calculating luminosity is Sass
    • Using luminosity, we now have functions to checkContrast, chooseLightOrDarkText and makeHighContrastColor.

In total, these new functions allow us to automatically set accessible safe colors when used together. This works regardless of theme, because it allows you to pass both a foreground and background color (which should be variablized through the theme).

Here's a simple example in .euiCallout

@each $name, $color in $callOutTypes {
  .euiCallOut--#{$name} {
    $backgroundColor: tintOrShade($color, 90%, 70%);
    
    // The below function gives us a text color that works against the tinted callout background.
    $textColor: makeHighContrastColor($color, $backgroundColor);

    border-color: $color;
    background-color: $backgroundColor;

    .euiCallOutHeader__icon {
      fill: $textColor;
    }

    .euiCallOutHeader__title {
      color: $textColor;
    }
  }
}

image

image

@snide snide requested a review from cchaos April 3, 2018 19:14

$highContrastTextColor: $forground;

@while ($contrast < 4.5) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Someone more engineery might have a better way to do this, but it seems to work perfectly fine and outputs decent looking results. Basically keeps adjusting the shadeOrTint by 5% of a passed color until it gets passed 4.5.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems simple enough to me. Not sure what the load is on compilation but at 5% increments I don't think it should run too long.

@@ -37,7 +9,7 @@ $euiColorGhost: #FFF !default;
// Status
$euiColorSuccess: $euiColorSecondary !default;
$euiColorDanger: #A30000 !default;
$euiColorWarning: #CF3800 !default;
$euiColorWarning: #E5830E !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.

This reverts back to our "yellow" color for warnings so everything looks ok again. It becomes darker through use of the functions as we need it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@snide
Copy link
Contributor Author

snide commented Apr 3, 2018

jenkins, test this

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.

This makes it sooo much simpler to just have contrast compliant colors! Awesome!

Overall, I would just like to see more explanation/examples of the SASS functions to know how to use them as it's not always super obvious without having to read through the code and understand what the function is doing.

@@ -0,0 +1,2 @@
@import 'math';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just add a comment in here that 'math' needs to come first to be used in all subsequent function files?

// Calculate contrast
@function contrastRatio($background, $foreground) {
$backgroundLum: luminance($background) + .05;
$foregroundLum: luminance($foreground) + .05;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why the + .05?

Copy link
Contributor Author

@snide snide Apr 3, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://www.w3.org/TR/2008/REC-WCAG20-20081211/#contrast-ratiodef

It's part of the formula for calculating contrast ratios when given relative luminance. To be honest, I barely understand how the math works, but I did check using @debug to make sure the value calculations along the functions work against WebAIM and other supported tools.

@return max($backgroundLum, $foregroundLum) / min($backgroundLum, $foregroundLum);
}

// Compare the contrast against both light and white text, then pick the highest.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compare against 'light and dark' or 'white and black'? I'm assuming it's not supposed to be 'light and white'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I think adding example usages in the comments would be really helpful.

// Given a color and a background, make that color AA accessibility by slightly
// adjusting it till it works
@function makeHighContrastColor($forground, $background) {
$contrast: contrastRatio($forground, $background);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sp: forground => foreground


$highContrastTextColor: $forground;

@while ($contrast < 4.5) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems simple enough to me. Not sure what the load is on compilation but at 5% increments I don't think it should run too long.

@@ -78,21 +78,16 @@ $buttonTypes: (
// Create button modifiders based upon the map.
@each $name, $color in $buttonTypes {
.euiButton--#{$name} {
color: $color;
color: makeHighContrastColor($color, $euiColorEmptyShade);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem to work for the ghost (non-fill) text color:

screen shot 2018-04-03 at 15 54 40 pm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

@snide
Copy link
Contributor Author

snide commented Apr 3, 2018

K, think this should be good to go. Added some more docs / examples.

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.

LGTM!

@snide snide merged commit cb878e2 into elastic:master Apr 4, 2018
@snide snide deleted the colors/warning branch April 4, 2018 01:03
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.

Cluster health is indicated as Red although the actual cluster status is yellow
2 participants