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

Allow coord grammar to be specified for the color syntax #370

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

lloydk
Copy link
Collaborator

@lloydk lloydk commented Nov 28, 2023

Some non CSS level 4 color spaces need coordinate grammars that go beyond what is currently allowed for the color() syntax.

For example the luv color space requires [-1, 1] to be specified for its u and v coordinates. Other color spaces such as hsluv, hpluv, okhsl, okhsv have coordinates that are angles.

Some non CSS level 4 color spaces need coordinate grammars
that go beyond what is currently allowed for the color() syntax.

For example the luv colorspace requires <percentage>[-1, 1] to
be specified for its u and v coordinates. Other color spaces
such as hsluv, hpluv, okhsl, okhsv have coordinates that are
angles.
Copy link

netlify bot commented Nov 28, 2023

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 65c7c5d
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/6566682526870d0008254828
😎 Deploy Preview https://deploy-preview-370--colorjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@svgeesus
Copy link
Member

Good, point, in CSS Color 4 the parameters of the color() function, after the initial color space identifier, are

[ <number> | <percentage> | none ]{3}

so specifying polar spaces inside color() is an unresolved problem.

@LeaVerou
Copy link
Member

Good, point, in CSS Color 4 the parameters of the color() function, after the initial color space identifier, are

[ | | none ]{3}

so specifying polar spaces inside color() is an unresolved problem.

Not something to resolve in this PR, but I think this should be fixed in the spec. We should allow <angle> | <number> | <percentage> and IACVT if needed (or define how each type resolves to the other two)

@LeaVerou
Copy link
Member

Hi there, apologies again for the delay, apparently I had seen this PR before, but it somehow completely fell off my radar.

I'm …not quite sure what this is doing. Abstracting that block of code to a separate function seems like a useful refactor, but beyond that I'm not quite sure what new use cases it's enabling. A coord grammar can already be specified for color(), by explicitly adding it as a format, and a range can already be specified in a coord grammar (e.g. <number>[1, -1]). Pretty sure I'm just not getting it, so if you could explain it a bit more, that would be useful! Sorry for the trouble @lloydk, especially after you've already waited so long!!

@lloydk
Copy link
Collaborator Author

lloydk commented Jan 20, 2024

Hi there, apologies again for the delay, apparently I had seen this PR before, but it somehow completely fell off my radar.

I'm …not quite sure what this is doing. Abstracting that block of code to a separate function seems like a useful refactor, but beyond that I'm not quite sure what new use cases it's enabling. A coord grammar can already be specified for color(), by explicitly adding it as a format, and a range can already be specified in a coord grammar (e.g. <number>[1, -1]). Pretty sure I'm just not getting it, so if you could explain it a bit more, that would be useful! Sorry for the trouble @lloydk, especially after you've already waited so long!!

Maybe I'm missing something but if you look at the current code the logic for handling the coord grammar is only executed for color space specific functions (oklab, oklch, lab, etc.).

@lloydk
Copy link
Collaborator Author

lloydk commented Feb 1, 2024

I think you might have missed the fact that the extracted code is now called in two places instead of one place in the original code.

You can currently specify a coord grammar for the color function but it isn't used and that's what this PR fixes.

Copy link
Member

@LeaVerou LeaVerou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, thanks! LGTM

@LeaVerou
Copy link
Member

LeaVerou commented Feb 1, 2024

One thing — I did not check if the code that was moved to the function was the same, did you make any changes there?

@lloydk
Copy link
Collaborator Author

lloydk commented Feb 1, 2024

One thing — I did not check if the code that was moved to the function was the same, did you make any changes there?

I didn't make any changes to the code in the extracted function

@LeaVerou LeaVerou merged commit e906469 into color-js:main Feb 1, 2024
4 checks passed
@LeaVerou
Copy link
Member

LeaVerou commented Feb 1, 2024

K, merged, thanks!

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.

3 participants