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

Implement tint and shade functions #953

Closed
wants to merge 3 commits into from
Closed

Conversation

Synchro
Copy link
Member

@Synchro Synchro commented Sep 12, 2012

Replacement pull request for the messed up #947 (see that issue for discussion) which fixes #944, adding tint and shade functions, with tests.

@ricardobeat
Copy link
Contributor

What I meant by inverting the amount (on #947) is this:

shade: function(color, amount) {
    return this.mix(color, this.rgb(0, 0, 0), 100 - amount);
}

That is, a tint(100%) of anything is white, and shade(100%) is black.

This is the approach taken by SASS too.

@lukeapage
Copy link
Member

It looks symmetrical to me already... Your change would make it asymmetric.. or am I missing something?

Put mix values in the right order, update tests to be clearer
…intshade

Conflicts:
	lib/less/functions.js
	test/css/functions.css
	test/less/functions.less
@Synchro
Copy link
Member Author

Synchro commented Sep 14, 2012

OK I see what you mean - the problem wasn't really inverting the amounts, it's that I'd got the order of parameters wrong in the call to mix - the amount parameter determines how much of the first colour is mixed with the second, not the other way around; that's now fixed. Also the test values chosen didn't make it obvious whether colors were getting lighter or darker, so I changed them to something more apparent, and added 100% cases.
I did squash these commits, but it still made me do a merge on push which was a little messy - I've also been reading that pushing rebases to public repos is frowned upon.

@lukeapage
Copy link
Member

not sure what the merge is about.. I just rebase and then push it to a branch with force (since it overwrites the history on the target branch if you don't it warns you as you are deleting stuff) I think that is your problem - you must be rebasing fine locally then merging to your public repo branch and essentially undoing the rebasing.

What is frowned upon is re-writing history on a branch which other people are using for rebasing and merging - for instance If I decided to rebase cloudhead/less.js - it would screw over everyone who had merged the commit I squashed and git would get confused.

AFAIK there is no problem rebasing a public history if it is your own repo that no-one has branched.

@Synchro
Copy link
Member Author

Synchro commented Sep 14, 2012

Yes, I need to force on push, thanks. Apart from that, is this pull request ok?

@lukeapage
Copy link
Member

In the wake of 9 days with no criticism I pulled it.

@lukeapage lukeapage closed this Sep 23, 2012
@Synchro
Copy link
Member Author

Synchro commented Sep 23, 2012

Thanks, Luke. You're doing a fantastic job BTW!

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.

Would love to add 'tint' to color functions
3 participants