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

mix implementation is not correct #60

Merged
merged 6 commits into from Jun 21, 2015
Merged

mix implementation is not correct #60

merged 6 commits into from Jun 21, 2015

Conversation

iamstarkov
Copy link
Contributor

these tests from libsass’s spec. For me they are not working in https://github.com/iamstarkov/postcss-color-mix

@iamstarkov
Copy link
Contributor Author

it doesn’t work for actual examples from libsass, like

a { color: mix(#f00, #00f); }
=> a { color: #800080; } /* which is identical to purple if you using http://sassmeister.com/ */

but color.mix is resulting #4000bf, which is wrong.

@iamstarkov
Copy link
Contributor Author

probably these failing tests are similar to #46

@MoOx
Copy link
Collaborator

MoOx commented Jun 18, 2015

What is the purpose of this PR. Will you fix those?

@iamstarkov
Copy link
Contributor Author

Purpose of this pr is a proof, that mix is broken right now. Mainly because issues like #46 only stating that there are some errors in implementations, while waisting your time to find negative test

@iamstarkov
Copy link
Contributor Author

Will you fix those?

Should I? If you have no time or will to do it, then yes, I will take it.
The question is, do you want to do it yourself?

@MoOx
Copy link
Collaborator

MoOx commented Jun 18, 2015

No time for this right now. Too busy with other stuff :)

@iamstarkov
Copy link
Contributor Author

okie-dokie, will do it

@iamstarkov
Copy link
Contributor Author

will close and re-open when will be ready

@iamstarkov iamstarkov closed this Jun 18, 2015
@iamstarkov
Copy link
Contributor Author

my fix is already done, but I have one question.

are you avoiding named tests and mocha as test runner?
Just for me in development it.only and endless watching are priceless features, so I a refactored test a bit. For testing these features clone my fork and run mocha or mocha -w in development mode. So the question is should I revert my changes before open pull-request or you are fine with mocha?
screen shot 2015-06-19 at 15 39 06

@iamstarkov iamstarkov changed the title add new tests for github and twiitter mix implementation is not correct Jun 19, 2015
@MoOx
Copy link
Collaborator

MoOx commented Jun 19, 2015

legacy from the original author of the module.

I use tape everyday, so I would like to see this one :)

But since I didn't do anything yet, I guess mocha is better than nothing. Feel free to push :)

@iamstarkov iamstarkov reopened this Jun 19, 2015
@iamstarkov
Copy link
Contributor Author

Done and tests are green!

@MoOx
Copy link
Collaborator

MoOx commented Jun 19, 2015

Do you confirm that this is a patch only ? (So I can release as a patch)

@iamstarkov
Copy link
Contributor Author

formally, it is a patch, because previous implementations was incorrect. I only properly ported C implementation

@iamstarkov
Copy link
Contributor Author

7,616 downloads in the last day

but, i’m afraid that this method can be used by somebody and they dont expect their site to be totally different after patch deps-bump

@iamstarkov
Copy link
Contributor Author

so probably it is backwards-incompatible patch, but im not sure

@MoOx
Copy link
Collaborator

MoOx commented Jun 19, 2015

Since we are in 0.x, I will release 0.x+1 :)

@iamstarkov
Copy link
Contributor Author

valid point!

@iamstarkov
Copy link
Contributor Author

as far as we switching to mocha, I updated instructions

MoOx added a commit that referenced this pull request Jun 21, 2015
mix implementation is not correct
@MoOx MoOx merged commit 734632c into Qix-:master Jun 21, 2015
@MoOx
Copy link
Collaborator

MoOx commented Jun 21, 2015

Does this fix #46 btw ?

@MoOx
Copy link
Collaborator

MoOx commented Jun 21, 2015

Released as 0.9.0

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.

None yet

2 participants