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

Add spaceKey property to Color.prototype #460

Closed
wants to merge 1 commit into from

Conversation

sidewayss
Copy link
Contributor

@sidewayss sidewayss commented Feb 28, 2024

Converts a color space id to a string key that can be used to access a Color instance's color space properties/values. Essentially, it converts kebab-case to snake_case. Example usage code:

const color1 = new Color("color(xyz-d50 3 1 23)");
const color2 = new Color("lab(23.3 -573 1140)");
color1.coords = color2[color1.spaceKey];          // spaceId = "xyz-d50", spaceKey = "xyz_d50"
// or conversely:
color1[color2.spaceKey] = color2.coords;

I use it to read actual html_element.style or getComputedStyle() values and convert them to my Color instance's color space. The workaround for not having it is simple, but IMO this is a conversion that the library itself should handle. If not, then never mind...

I have created the property in Color and not in ColorSpace because it's used by the Color instance, not the space. But if you prefer, it could be added as ColorSpace.protype.key instead of or in addition to Color.prototype.spaceKey.

The name spaceKey is certainly open to change.

Super simple code, and I've tested it locally, but I can't figure out a good way of creating a public test. Github pages won't build the repo. Here are the pair of HTML/JavaScript files I used to test it, if that helps (can't attach .html or .js files here and the path to color.js requires editing for your local environment):
https://sidewayss.com/test/testColor1.html
https://sidewayss.com/test/testColor1.js

I think I got it right that because this is a change to the color.js module I should not put any "[text]" in the commit message or PR title. Let me know if I got that wrong.

Converts a color space id to a string key that can be used to access the Color instance's color space values.  Essentially, it converts kebab-case to snake_case.
Copy link

netlify bot commented Feb 28, 2024

Deploy Preview for colorjs ready!

Name Link
🔨 Latest commit 2219f27
🔍 Latest deploy log https://app.netlify.com/sites/colorjs/deploys/65df7f79d46416000869e00e
😎 Deploy Preview https://deploy-preview-460--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 LeaVerou added the API change For PRs that require API design review label Feb 28, 2024
@LeaVerou LeaVerou self-requested a review February 28, 2024 22:49
@LeaVerou
Copy link
Member

Hey, thanks for submitting a PR!

I’m not quite sure if this use case is worth solving at the API level: if you need to get coords of an arbitrary color space, you can always use getAll() which accepts both color space objects and color space ids.
E.g. your example would become:

const color1 = new Color("color(xyz-d50 3 1 23)");
const color2 = new Color("lab(23.3 -573 1140)");
color1.coords = color2.getAll(color1.space);
// or conversely:
color1.setAll(color2.space, color2.coords);

which arguably is easier to read.

@sidewayss
Copy link
Contributor Author

Thanks for explaining getAll() and setAll() and providing an example. The online docs don't cover those functions. That solves my problem. Given your understandable lack of enthusiasm, I'll close this PR.

Is there an other way or place to ask questions before submitting a PR? I was looking for a way to set a Color instance's coordinates using a string, they way you can when you instantiate a Color. Is there a way to do that via some subset of the parse() function? The idea is to workaround new Color() because this is a library with no dependencies (like Color.js). I am working around it with new colorInstance.constructor() and passing in the string that way. But if there's a setAll() that parses already I'd love to know about it.

Thanks!

@sidewayss sidewayss closed this Feb 28, 2024
@facelessuser
Copy link
Collaborator

You can ask questions in the discussions tab of this repo: https://github.com/color-js/color.js/discussions.

@sidewayss
Copy link
Contributor Author

You can ask questions in the discussions tab of this repo: https://github.com/color-js/color.js/discussions.

Thanks, I just asked my question there.

@LeaVerou
Copy link
Member

@sidewayss could you open an issue about the lack of docs? That's definitely something we should fix!

@sidewayss
Copy link
Contributor Author

sidewayss commented Feb 29, 2024

#463 is the new docs issue.

Note that I realized after closing this PR that my problem still persists because my description of the use case was over-simplified for purposes of describing it briefly. But it's all related to my discussion Q&A topic, so I'm hoping it will sort itself out there. It does seem that the API itself should provide this conversion, though it's highly unlikely that the property names will ever change and very few users will use it. I have attached a pdf matrix (original is in Excel) of the three versions of color space names that Color.js supports: CSS, pseudo-css tokens for color() (e.g. --a98-rgb-linear), and Color.prototype.property. The conversion rules are not 100% consistent, so the matrix is useful. Maybe the matrix should be in the docs somewhere?

color space names.pdf

@LeaVerou
Copy link
Member

These are not arbtrary at all, but I agree the logic should be published in the docs. Could you open an issue about that too?

@sidewayss
Copy link
Contributor Author

sidewayss commented Feb 29, 2024

#465 is the new docs issue for color space names. I attached the pdf and its Excel source (I hadn't realized that I could attach .xslx files).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change For PRs that require API design review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants