-
Notifications
You must be signed in to change notification settings - Fork 82
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
Allow any color type when calling individual deltaE functions #451
Conversation
✅ Deploy Preview for colorjs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TypeScript changes LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree on solving the problem, not sure about this particular solution, I really don't like the repetition. If there are other commonalities between the deltaE functions, we could make this part of a utility function from deltaE.js
that these methods import.
If there aren't other commonalities, maybe a better solution is to make getColor()
accept a variable number of arguments so it can become a single line:
let [sample, color] = getColor(sample, color);`
I don't think there are any commonalities between the deltaE functions. The contrast functions have the same repetition. Lines 41 to 43 in 1281ad1
What about adding a export function getColors (...colors) {
return colors.map(getColor);
} I don't have a strong opinion on any of the solutions:
If we want to make changes to |
How about this option: instead of variable arguments to Pros:
It does add two extra chars to usage, but I think that's an ok tradeoff, and it makes it more explicit. The advantage of extending |
Created PR #453 for When you say Lines 111 to 121 in 52d5f69
|
Ah, I thought it was using |
The types for the deltaE functions didn't allow all ColorTypes as parameters. For example, the types didn't allow "red" to be passed as a parameter. Also, calling a deletaE method with a color such as "red" would cause an exception to be thrown. e.g. let x = deltaE2000("red", "white") // Exception thrown
Messed up my branch. Hopefully I can fix it without making more of a mess. |
4103bf8
to
b96472a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
The types for the deltaE functions didn't allow all ColorTypes as parameters. For example, the types didn't allow "red" to be passed as a parameter.
Also, calling a deletaE method with a color such as "red" would cause an exception to be thrown.