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

Replace yiq contrast by wcag official algorithm #22996

Closed
wants to merge 11 commits into from

Conversation

Lausselloic
Copy link
Contributor

Following @mdo PR for color update : #22836

Many thanks for the color contrast mixin which check the font color according to the background for badges only at time. But yiq is not the official wcag and doesn't give the same result on some colors (e.g with blue #527edb yiq set text color to white where official wcag lum set it to black.

I propose a SCSS adaptation of the HTMLCS JS solution. Need to add a dependency to sass-math-pow.

I hope this solution will be helpfull. Let me know

@Johann-S
Copy link
Member

Johann-S commented Jul 5, 2017

you should update our shrinkwrap because you added a new dependency with npm run maintenance-shrinkwrap

@Lausselloic
Copy link
Contributor Author

Yes I've seen it. I will update till 1hour

@@ -53,13 +56,28 @@
$r: red($color);
$g: green($color);
$b: blue($color);
// get the relative lum for each color
$luminositecouleur: relativelum($r / 255, $g / 255, $b / 255);
Copy link
Member

Choose a reason for hiding this comment

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

small request: as the official language for our files is english, could you update the variables to $luminositycolor and $luminositywhite ? merci :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes

Copy link
Member

Choose a reason for hiding this comment

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

actually, to stay consistent with current WCAG wording (though from what I remember there's discussion about it being incorrect), this should be $luminancecolor and $luminancewhite

Copy link
Member

Choose a reason for hiding this comment

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

sorry, you were quicker than my brain which needed more coffee ;)

@Lausselloic
Copy link
Contributor Author

No problem I've many coffee :-)

@patrickhlauke
Copy link
Member

LGTM but I'm not a SASS expert. @mdo et al, feel free to merge if this looks kosher to you.

@pat270
Copy link
Contributor

pat270 commented Jul 5, 2017

Would be nice to have @mixin color-yiq() be a function that returns a value instead of printing a css declaration so we could use it in Sass variables.

@Lausselloic
Copy link
Contributor Author

yes, that's my final idea, evolve that mixin to create a generic one that return a comparison between two colors. Or add in parameter output colors

@mdo
Copy link
Member

mdo commented Jul 5, 2017

Need to add a dependency to sass-math-pow.

Not a fan of the dependency here. Is there another solution we could try? And if this changes what is output on our components, we'll need to revisit more things.

@patrickhlauke
Copy link
Member

if this changes what is output on our components, we'll need to revisit more things

I believe functionally the output is the same, just that it flips between light and dark text at a better calculated threshold which matches up with WCAG 2's requirements.

@mdo
Copy link
Member

mdo commented Jul 5, 2017

Yeah, I mean visually as much as the compiled code :).

@patrickhlauke
Copy link
Member

Visually it'll be the same, except at the point just on the cusp between where light or dark would be better. There, it'll flip at a slightly earlier/later point, so yes for certain background colors it will be visually different.

@Lausselloic
Copy link
Contributor Author

I understand and agree that adding a sass dependency is not fun. For @mdo the solution could be to Copy/paste source code directly in bootstrap utilities, because I don't think that pow expression will change in the time.
For visual consistence, would be possible to add optionnal input like :
color-yiq($color, $chalenging-color-list, $light-fallback, $dark-fallback) {
Where $color will be the font or background color to check against an ordered color in $chalenging-color-list, and if none are valid then return light or dark fallback ?

for each $color in $chalenging-color-list -> check if ratio is valid, return the first valid color
if none have been return, check if light or dark fallback is better

@razh
Copy link
Contributor

razh commented Jul 6, 2017

One alternative to the extra dependency would be to use the experimental functions feature in node-sass.

const sass = require('sass')

const functions = {
  pow(base, exponent) {
    const result = Math.pow(base.getValue(), exponent.getValue())
    return new sass.types.Number(result)
  }
}

Would need some additional documentation for ruby-sass and other variants.


// Export math.pow function for node-sass

var types = require('node-sass').types
Copy link
Member

Choose a reason for hiding this comment

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

you should use const instead of var here because you don't change types

@Lausselloic
Copy link
Contributor Author

Hi, any news about this PR? I can update it if needed

@pierre-hilt
Copy link

Hello, do you have news about this issue? We would like to use WCAG contrast for color-yiq function. We tried to play with the treshold, but as it ia not the same function we can't have something perfect.

@Lausselloic
Copy link
Contributor Author

Hello,

I use it in Boosted (that's why I create this PR) But wonder that one will go into the core soon.

@XhmikosR
Copy link
Member

XhmikosR commented Nov 4, 2018

@Lausselloic: can you fetch upstream and rebase?

@MartijnCuppens
Copy link
Member

I'm afraid we can't do this. This will make bootstrap not buildable with anything else than node-sass and it will require everyone to adapt their build with this custom config.

@mdo
Copy link
Member

mdo commented Dec 14, 2018

@MartijnCuppens Damn, if that's the case, feel free to close this out. Will wait for you to confirm though!

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

Successfully merging this pull request may close these issues.

None yet

9 participants