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

parse.js fixes to allow "custom" ColorSpace formats #595

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sidewayss
Copy link
Contributor

@sidewayss sidewayss commented Sep 7, 2024

Two very simple fixes that come out of this discussion: #593

  1. The code was finding a match for a custom parse() function, but only exiting the inner of two loops. I added a new variable: found, that the inner loop sets to true so that the outer loop knows to exit. This was not only causing unnecessary iterations of the loop, it was causing issues with custom user ColorSpaces that have formats with {type:"custom"}.
  2. The addition of a single ? in this line: let color = format.parse?(env.str); so that "custom" ColorSpace formats don't require a parse property. The discussion linked above creates a custom ColorSpace with a default format that has a custom serialize() property/function, but doesn't need a parse property. The current code requires it to have a dummy parse() property/function that returns falsy. This change removes that requirement and prevents the throwing of unnecessary errors.

It is worth noting that this outside loop on ColorSpace.all is inefficient relative to its contents, even after fix 1. There is only one built-in space with custom formats: sRGB, and it's the near the end of ColorSpace.all. I'm no expert, but based on what I've seen I can't imagine any other built-in spaces doing custom parsing. As for user color spaces, I don't know if any other users have created any, and mine doesn't parse. Are there pending proposals for new CSS color strings that parse in other spaces? Is there a need for a user color space that parses a custom, non-CSS color string?

If you want to ensure that user spaces and new built-in spaces can do custom parsing, you can create a new ColorSpace.custom array containing sRGB all other registered spaces that have custom formats. You could add spaces to that array in register(). Otherwise it's just sRGB, no need to iterate over any other color spaces.

Copy link

netlify bot commented Sep 7, 2024

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 62ee351
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/66db9f74edebba0008f6f890
😎 Deploy Preview https://deploy-preview-595--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.

@LeaVerou
Copy link
Member

Oh wow, this completely fell through the cracks, so sorry @sidewayss! Do you want to rebase so we can review? I think the type info in this is out of date, which is why it's showing so many type issues.

@LeaVerou
Copy link
Member

Btw if the inner loop needs to break the outer loop, you can do that with labels: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/label

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.

2 participants