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

Remove alpha from contrast calc #1733

Merged
merged 1 commit into from
Dec 18, 2013
Merged

Remove alpha from contrast calc #1733

merged 1 commit into from
Dec 18, 2013

Conversation

Synchro
Copy link
Member

@Synchro Synchro commented Dec 17, 2013

#1724 highlighted a misunderstanding of how the contrast function works. I'd thought of this at the time I wrote the contrast function, and I'd incorporated the alpha value into the calculation. This is evidently wrong, because we can't know the background colour, so we have no idea whether the alpha will lighten or darken the colour it's comparing. So the alpha term should be removed from the calculation altogether. While this will affect current users, current output is actively wrong so I don't think it's a BC break, just a bug fix.

@seven-phases-max
Copy link
Member

+1
It probably can even throw a warning when transparent color is used as contrast argument(s) (if we'll have a "warning facility")

@lukeapage
Copy link
Member

we have the start of a warning facility...

@seven-phases-max
Copy link
Member

@lukeapage do you mean console.warn like here? Yes, I guess this is the start, though it still needs some goodies to show line/column it is rised from (or do I miss something?).

@Synchro
Copy link
Member Author

Synchro commented Dec 17, 2013

Good idea - separate ticket though? OK to merge this?

@lukeapage
Copy link
Member

@seven-phases-max yes.. it really is a tentative start!

lukeapage added a commit that referenced this pull request Dec 18, 2013
Remove alpha from contrast calc
@lukeapage lukeapage merged commit 7a5e432 into less:master Dec 18, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants