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

Fix desaturation of greyscale colours #865

Merged
merged 1 commit into from
Feb 2, 2015
Merged

Fix desaturation of greyscale colours #865

merged 1 commit into from
Feb 2, 2015

Conversation

anlutro
Copy link
Contributor

@anlutro anlutro commented Jan 30, 2015

See issue #864

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 76.12% when pulling c8ff945 on alprs:fix_desaturate-grey into 714484e on sass:master.

@QuLogic
Copy link
Contributor

QuLogic commented Jan 31, 2015

It looks like the related functions also don't clamp results, e.g., saturate does not limit the result to 100. Maybe you could add those fixes too.

@mgreter
Copy link
Contributor

mgreter commented Feb 1, 2015

@anlutro Could you come up with a test case which includes all edge cases? If so please add another PR to https://github.com/sass/sass-spec!

@anlutro
Copy link
Contributor Author

anlutro commented Feb 1, 2015

@QuLogic I did that originally but thought it might be misleading because it is originally the saturation of the colour passed to the function.

As for clamping saturate(), it actually seems to be working fine - saturate(red, 50%) still returns red.

saturate() on grey colours behaves weirdly in my opinion, but it behaves weirdly in ruby sass so I suppose it'd be bad form to try and change it in libsass.

I'll see if I can make a PR to the spec.

@xzyfer
Copy link
Contributor

xzyfer commented Feb 2, 2015

Spec added sass/sass-spec#249

@xzyfer
Copy link
Contributor

xzyfer commented Feb 2, 2015

@anlutro I've merged your specs which now causes this PR build to fail. Could you please investigate?

https://travis-ci.org/sass/libsass/jobs/48892884

@anlutro
Copy link
Contributor Author

anlutro commented Feb 2, 2015

Looks like I was wrong about saturate() not having the same issue. I can fix that as well I suppose.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) to 76.21% when pulling 9d2f331 on alprs:fix_desaturate-grey into 4d5ea59 on sass:master.

if (hslcolorS <= 0) {
hslcolorS = 0;
} else {
hslcolorS += amount->value();
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, this is += now for desaturation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yikes, stupid mistake. Glad you spotted it.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) to 76.21% when pulling b6b1d2e on alprs:fix_desaturate-grey into 4d5ea59 on sass:master.

@anlutro
Copy link
Contributor Author

anlutro commented Feb 2, 2015

OK, there is one more problem I have to fix - desaturate(saturate(#999, 1%), 10%); should return #999999 but returns #90a2a2.

@anlutro
Copy link
Contributor Author

anlutro commented Feb 2, 2015

Fixed and squashed commits, everything should be OK now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.12%) to 76.21% when pulling 657486b on alprs:fix_desaturate-grey into 4d5ea59 on sass:master.

@xzyfer
Copy link
Contributor

xzyfer commented Feb 2, 2015

Great work @anlutro

xzyfer added a commit that referenced this pull request Feb 2, 2015
Fix desaturation of greyscale colours
@xzyfer xzyfer merged commit 175c167 into sass:master Feb 2, 2015
@xzyfer xzyfer added this to the 3.2 milestone Feb 2, 2015
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.

5 participants