Add hwb() + fix max for hue #25
Conversation
Also, capping must be done before convert loop for each spaces. Otherwise it’s not coherent with how color-convert handle overflow (modulo). Ref #24
|
+1 |
| "cmyk": ["cyan", "magenta", "yellow", "black"] | ||
| }; | ||
|
|
||
| var maxes = { | ||
| "rgb": [255, 255, 255], | ||
| "hsl": [360, 100, 100], | ||
| "hsv": [360, 100, 100], | ||
| "hsl": [359, 100, 100], |
There was a problem hiding this comment.
It's pretty standard to allow 360 in color pickers at least. So, leave it 360.
There was a problem hiding this comment.
Can't do that. Like I said, it currently create a bug due to how color-convert transform 360 to 0.
So you can have hsl channel to hue = 0 & hwb channel set to 360. Seems not coherent to me.
There was a problem hiding this comment.
I can set 360 if you insist, but it create a bug that need to be fixed asap.
There was a problem hiding this comment.
For example if I set 360, this test https://github.com/harthur/color/pull/25/files#diff-e2686e2925145c11799d638b2f4706dcR84 will work with 0, not 360 (has "expected")
There was a problem hiding this comment.
Yep, leave it at 360. We can work out the inconsistency later perhaps. I think an overflow like 400 should become 360.
There was a problem hiding this comment.
Before doing that, I just want to be sure that you understand my previous comment.
If I do that I'll have to add a weird test
assert.equal(Color({h: 400, s: 50, l: 10}).hue(), 360); // existing test
// ... later
assert.equal(Color({h: 400, w: 50, b: 10}).hue(), 0);
// 0 ? Yes. Because `hue()` use hsl: hwb channel have been correctly set to 360,
// but conversion changes hsl hue to 0.Note: conversion responsible for the 360 -> 0 https://github.com/harthur/color/blob/72efbc775e97e649c00403d425888fd9d4e27bfa/color.js#L343
Are you sure you want that ?
There was a problem hiding this comment.
Aha, I see what you're saying. I think it's better than capping at 359. In that case 360 would be changed to 359 and I think that would be unexpected. http://hslpicker.com goes up to 360, as do many other color pickers I've seen.
There was a problem hiding this comment.
I'll push that changes but I deeply think we should just drop "capping", and handle the angle value like it can be: modulo 360. I mean that 720deg is a valid hue for hsl & friends.
There was a problem hiding this comment.
I think I agree that we should do modulo for hue.
|
I just updated the PR with updates to color-string & color-converter. Tests are ok with the PR (without npm hacks :p ) |
|
@harthur I've updated the PR & reset max to 360 :) |
|
Merged, thanks for the work and discussion! |
Following
Qix-/color-string#8
Qix-/color-convert#10
capping must be done before convert loop for each spaces.
Otherwise it’s not coherent with how color-convert handle overflow
(modulo).
Ref #24