Add CSS toGamut algorithm #344
Conversation
|
Time to un-draft this PR? I see there are comments from @jgerigmeyer which should be addressed |
|
@svgeesus Marked as ready for review. Between @jgerigmeyer and I, all of the outstanding comments are resolved. I'll note that there aren't tests for this yet. I see tests are currently transitioning, but I'm happy to add some tests. Although, I'd be likely using my implementation as both the source of truth and the system under test, as I'm not aware of a source for confirmed outputs from the algorithm. |
| if (coord < spaceCoord.range[0]) { | ||
| return spaceCoord.range[0]; | ||
| } | ||
| if (coord > spaceCoord.range[1]) { | ||
| return spaceCoord.range[1]; | ||
| } |
There was a problem hiding this comment.
Non-blocking: It feels this logic can be simplified, but couldn't come up with a refactor off the top of my head.
There was a problem hiding this comment.
I considered a clamp function like return Math.min(Math.max(coord, spaceCoord.range[0]), spaceCoord.range[1]);. I went with clarity over brevity, but could easily switch.
There was a problem hiding this comment.
If a clamp function is correct (they don't 100% have the same semantics as this code), I think the best course of action is to define a clamp(min, value, max) in util.js, and then use it here.
Right, that has been a problem for a while. Safari has the CSS GMA in there (from when it was needed for conversion to HSL or HWB) but it might be an older iteration. Testing framework is now stable, and easier to write to without all the copy-paste boilerplate that we had previously. |
* main: Add CSS toGamut algorithm (color-js#344) Accept "Lightness" for lab space first channel name (color-js#348) Fix type def for MixOptions (color-js#347) Add CJS file to /fn entry and fix legacy builds (color-js#349)
|
|
||
| // The reference colors to be used if lightness is out of the range 0-1 in the | ||
| // `Oklch` space. These are created in the `Oklab` space, as it is used by the | ||
| // DeltaEOK calculation, so it is guaranteed to be imported. |
There was a problem hiding this comment.
I'd use a reference to the color space then. Using string ids requires color space registration, which breaks tree-shaking. Can't remember if we do something special for basic color spaces like this one, but I don't think so,
|
@LeaVerou I'll open a new PR. I'm also seeing an issue with the code, where It appears that |
This implements the CSS Color 4 Algorithm, as demo'ed in #154, and specified in https://drafts.csswg.org/css-color/#css-gamut-mapping.
Remaining: