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

Alter the mix function to make the weight behave the way the code looks #117

Merged
merged 2 commits into from Jun 29, 2017

Conversation

tsdorsey
Copy link
Contributor

@tsdorsey tsdorsey commented Jun 29, 2017

Hi. This came up when I tried to do this:

const color = Color(baseColor)
  .mix(Color(hoverColor), values.hover)
  .mix(Color(activeColor), values.active)
  .mix(Color(errorColor), values.error);

values is an object that contains 0 to 1 values for the hover, active, and error state which are animated. My thought was that if you start with the baseColor and "mix" the others in with 0 weight I would be left with the base color. Then as the weights animated to 1 I would end up with the full state color.

Turns out that's not what happens, a weight of 0 overwrites the original color while 1 will keep it and use none of the passed in color. I've looked at the code and documentation you cited as the source of the function and it makes sense now.

In the original code it's a function, not a method on an object. So when you write mix(black, white, 0.1);. You read that as "mix black into white by 10%". Contrast that with this module where the mix function is a method on a color object. When you write white.mix(black, 0.1) it is more natural to read it as "starting with white, mix in black by 10%".

You could argue that it's just me being biased towards reading the .mix method as "mix in" when I should have read it as "mix into". My counter to that would be that all of the other alteration methods (.whiten, .blacken, .fade, etc) return the original value when you pass 0 as the amount you want to alter them by. .mix on the other hand will return same things only when 1 is passed in. Also of note is that one of the examples in the README file is this: color.mix(Color("yellow"), 0.3) // cyan -> rgb(77, 255, 179) which is the result you get from my alterations but is not the value you'll get with the current mix function.

In conclusion, I know that could simply write my code as

const color = Color(errorColor)
  .mix(Color(activeColor), values.error)
  .mix(Color(hoverColor), values.active)
  .mix(Color(baseColor), values.hover);

but that is WAY more difficult to parse than the example I gave at the start.

In this change I...

  • Switched the colors in mix method
  • Updated the mix tests
  • Added tests for 25% and 75% mixes
  • Bumped the version to 2.0.0 (breaking change)

@Qix-
Copy link
Owner

Qix- commented Jun 29, 2017

Wow, I completely agree. I'm not sure why I didn't catch this when I did the makeover PR.

Could you revert the package.json version bump? I'll do that separately.

Thanks for bringing this to my attention!

@Qix-
Copy link
Owner

Qix- commented Jun 29, 2017

Also, could you update the readme? :)

@tsdorsey
Copy link
Contributor Author

I'll undo the version bump. What about the readme needs updating?

@Qix-
Copy link
Owner

Qix- commented Jun 29, 2017

The part about the .mix function (just search in-page .mix and you'll see it). The comments that outline the expected output should have their elements fixed.

@tsdorsey
Copy link
Contributor Author

Oh, yeah. I mentioned that in my essay up there (didn't have time to make it shorter).

The only affected example color.mix(Color("yellow"), 0.3) // cyan -> rgb(77, 255, 179) is NOW going to be correct. ;)

@Qix-
Copy link
Owner

Qix- commented Jun 29, 2017

Oh, derp, you're right. It's late and I'm battling with CI things over at chalk 😅.

Thank you so much for the investigation and the fix - essays are always appreciated ;)

@Qix- Qix- added the bug label Jun 29, 2017
@Qix- Qix- merged commit 5f1ea40 into Qix-:master Jun 29, 2017
@Qix-
Copy link
Owner

Qix- commented Jun 29, 2017

Released as 2.0.0. Thanks again :)

@tsdorsey
Copy link
Contributor Author

My pleasure.

@kumarharsh
Copy link

This PR really should have had an accompanying changelog. I upgraded color, and now have to change all the places I was using mix.

Plus, I had just come to terms with mix behaving oddly, when I find that the implementation was finally fixed. 😖 😉 Need to rewire brain again.

BTW, good work on fixing the API @tsdorsey 👏.

@kumarharsh kumarharsh mentioned this pull request Nov 8, 2017
@tsdorsey
Copy link
Contributor Author

tsdorsey commented Nov 9, 2017

@kumarharsh You're right. Thanks for reminding me.

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.

None yet

3 participants