Closed bluesaunders closed 11 years ago
I don't use the contrast module, but lately I keep thinking I should. This seems to be a clear improvement in the results. Well done!
I think the core team would welcome a pull request :)
Thanks! I'd like to see a little discussion around this before I make a pull request, but if it's well-received, I'd be more than happy to make my first contribution to Compass :)
Here is a slightly cleaner version of the example that more closely follows the structure of the existing color contrast module. http://codepen.io/bluesaunders/pen/bEkuh
Edit: i did some digging and discovered that originally the the contrast utility did something similar to what I'm doing (using the same "YIQ" algorithm) but was later switched to using a lightness threshold instead. see here
The technique I'm using is a bit different than the original in that, instead of checking against a set range, I look at the absolute values of the differences in contrast and choose the higher number.
I would welcome this on the grounds that its not a replacement and is another option mostly for regression purposes
@scottdavis If this mixin produces better results (we'll need more tests) and we add this, then I think the existing mixin should be deprecated. Doesn’t make sense to indefinitely maintain both.
If they accomplish the same task, one better than the other, why not simply replace the old with the new?
Look at the use case where someone has built a style guide off the old one. If we replace it in a new version there design changes and it will be a nightmare. Deprecation is fine by me I just didn't want to pull the rug out.
Maybe I mis-understand this do they produce the same color values?
No, this uses a different algorithm to choose which colors to put together, so output sometimes differs from the existing mixin.
I think a better illustration of this can be seen by changing the light and dark values to something other than black & white.
For illustrations sake, let's change the dark default to blue, and the light default to yellow.
Example One: current implementation Example Two: newly proposed version
As you can see, the blue on the first example (far right on row 1) chooses a blue text, but in the second version, the more contrasty yellow is chosen.
Now, keep in mind that this doesn't suggest that the colors are well contrasted, just that the function selects the most contrasted of the given light and dark values.
For instance, in Example Two: blue text on a cornflowerblue background is not a truly suitable contrast, but it's better than yellow text on a cornflowerblue background. As evidenced here, and here.
@scottdavis as far as producing the same color values, that's not the case here. In this example any of the white text values that show a Lightness of above 30%, would be black using the current "contrasted" method.
@scottdavis Also, I can totally understand not wanting to pull the rug out from under something already established. Are there any usage numbers for the contrasted method? (Not sure if this is even measurable.)
Perhaps someone who used the color contrast utility could chime in?
We use it at work and we have come to use those colors in other areas. So it directly effects me that why I was hesitant. In reality i just have to change one variable but for the un knowing this could be an issue.
just being the devils advocate here btw
@scottdavis Makes sense.
What is the best course for contributing this with the goal of deprecation on the current version?
I also use it, but I would welcome this change. If I'm using the contrast mixins, that's because I want the best contrast between two options. If the current mixin is getting that wrong, I want it to be replaced, and I want those changes to propagate on all my sites. I see the changed output as an advantage.
Also, I'm fine with a total replacement if it goes out with 0.13. If this is intending to go out wit ha 0.12.x it needs to be named something else or it needs to wait. I don't like making an functionality change in a non major release.
Oh yes. I guess I was assuming 0.13. Agree completely.
Sorry, I've been away for a bit... Should I go ahead and create/commit this? If so, can someone point me toward the best way to contribute this?
Please do make a pull request for this i would like to see it added for 0.13
Sorry, forgot about this again. I'd love to contribute this, but I'm hitting a problem.
When I try to run the tests, or when I try to point to my local build of compass to test in a real project I get the following error:
There was a NoMethodError while loading compass.gemspec: undefined method today' for Date:Class from <redacted path>/personal/compass/compass.gemspec:6:in
block in
Perhaps I missed something when setting up my local build?
@bluesaunders I don't know the test issue, sorry. Just wanted to chime in that I've been happily using your technique for the last 6 months. Thanks! I also did some work on the api, if you want to take a look at that:
https://github.com/ericam/accoutrement/blob/master/stylesheets/accoutrement/_color.scss
brightness
and contrast
are the two functions related to this module.
@ericam holy crap! this was opened 6 months ago? Time flies!
Glad to hear that the techinque has been working for you. You made some really nice changes to the api.
If I can't get an answer to the test situation, would you be able to contribute the new contrast code?
I can help you with the test problem my ghat is jetviper21 at gmail
Sent from my iPhone
On Mar 29, 2013, at 11:26 PM, Brendan Saunders notifications@github.com wrote:
@ericam holy crap! this was opened 6 months ago? Time flies!
Glad to hear that the techinque has been working for you. You made some really nice changes to the api.
If I can't get an answer to the test situation, would you be able to contribute the new contrast code?
— Reply to this email directly or view it on GitHub.
@scottdavis I got your message on the Google Group, and checked out master. My tests are now green (after altering them to the correct values.) Hooray, and thanks!
I'm now trying to make sure the documentation is fitting for the changes. I tried following the doc readme and ran into an issue in this step which I solved by running ./bin/nanoc aco
instead of ./bin/nanoc3 aco
. Now I'm running into the following in the browser:
I have exactly the same experience with the docs. I'm afraid the doc documentation may be out of date. :/
Alright I'll look into it this evening at some point
Sent from my iPhone
On Mar 30, 2013, at 1:38 AM, Brendan Saunders notifications@github.com wrote:
@scottdavis I got your message on the Google Group, and checked out master. My tests are now green (after altering them to the correct values.) Hooray, and thanks!
I'm now trying to make sure the documentation is fitting for the changes. I tried following the doc readme and ran into an issue in this step which I solved by running ./bin/nanoc aco instead of ./bin/nanoc3 aco. Now I'm running into the following in the browser:
— Reply to this email directly or view it on GitHub.
@scottdavis Did you ever get a chance to look into this? Would love to make sure documentation is updated before contributing the contrast change.
The docs work for me now. I had to pull the latest, then run bundle install
and ./bin/nanoc aco
.
@ericam Thanks for the heads up, I'll try that in a bit.
I fetched and merged the latest stable into my branch, but after a bundle install
and a ./bin/nanoc aco
I'm still getting this:
I don't know about stable. I was assuming you would pull this into master. I think that's the best place for it.
I deleted my local clone of the repo and started fresh. I get the same error even from master. Starting to get pretty discouraged :(
Yeah, damn, that's rough. What version of Ruby?
Why don't you push what you have, and I'll check the docs here and make any needed changes?
I didn't change anything but the docs started working. The only change I could see that needed making was for the example. Feels good to finally get this contributed. Sorry it took so long :/
\o/ Thanks!
No worries, glad to have contributed.
Hello,
I recently tweeted about a color contrast mixin that I authored based on a W3C algorithm for color contrast. (Article link...) I was asked to make an issue here, discussing the technique.
Compass's color contrast module seems to favor dark text because it depends on the HSLA lightness of a color. Selecting between light and dark text based on a lightness threshold is not enough to make proper color contrast decisions.
I've used the algorithm here for finding the difference between a given background color, and specified light, and dark values. The mixin will use the light or dark value that has a greater difference in contrast form the background color.
So, instead of relying on a threshold of lightness, the result here is that you always get the value that is higher contrasted with the background color.
NOTE: The goal was to provide the most contrasted color between the light and dark values supplied. So it is possible to have low contrasting colors, if the light or dark values are not much different than the background.
You can see my results here.
PS. I left the compass "contrasted" mixin commented in the code, un-comment to see the difference between contrast choices.