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

[color-swatch] Add the colorInfo prop #118

Merged
merged 6 commits into from
Oct 16, 2024
Merged

[color-swatch] Add the colorInfo prop #118

merged 6 commits into from
Oct 16, 2024

Conversation

DmitrySharabin
Copy link
Member

@DmitrySharabin DmitrySharabin commented Oct 11, 2024

As we discussed in #96, the color info is also helpful to get programmatically. This PR addresses this. Under the hood, the data structure suggested by @LeaVerou (Option 1) is used.

This PR is the first step to implementing deltas and contrast and is intended to check if I got the main idea shaped in the PR mentioned right. The existing API is the same, so it's not a breaking change. Everything still works as before.

The gist of what this PR introduces can be seen in the screenshot (<color-swatch> is a Lego block for color charts):

SCR-20241011-peso

Make the color info available programmatically.
Copy link

netlify bot commented Oct 11, 2024

Deploy Preview for color-elements ready!

Name Link
🔨 Latest commit a92705c
🔍 Latest deploy log https://app.netlify.com/sites/color-elements/deploys/670ea5dcf7a93300085d0101
😎 Deploy Preview https://deploy-preview-118--color-elements.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.

@DmitrySharabin
Copy link
Member Author

DmitrySharabin commented Oct 11, 2024

I'd like to discuss what is more helpful (for our users) to expose as colorInfo keys: the coords labels or channels themselves?

Option 1

{
    Lightness: 0.9158,
    Chroma: 0.05112,
    Hue: 349.9
}

Option 2

{
    "oklch.l": 0.9158,
    "oklch.c": 0.05112,
    "oklch.h": 349.9
}

@LeaVerou
Copy link
Member

I would vote for the keys, the labels might even be localized or have weird characters.

@DmitrySharabin
Copy link
Member Author

I would vote for the keys, the labels might even be localized or have weird characters.

Thank you. I'm leaning toward that, too. Let me update the code, then.

@DmitrySharabin
Copy link
Member Author

DmitrySharabin commented Oct 11, 2024

Done. Here is an updated gist of the proposed changes (in one screenshot):

image

@@ -189,6 +180,35 @@ const Self = class ColorSwatch extends ColorElement {
},
dependencies: ["color"],
},
infoLabels: {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need a separate infoLabels prop?

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially, I was thinking of optimizing the code so that we don't have to use Object.entries every time we need to get a channel and its label. It turned out that this was not a good idea: actually, we needed to do this in one place, so there was no need to add an extra prop. I removed it. Thank you for the question; it helped me see the flaw.

That was not a good idea: it's not that much reusable as it seemed to me from the first sight
We should round the result when using it in the UI
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.

LGTM

@DmitrySharabin DmitrySharabin merged commit 02e5e6f into main Oct 16, 2024
4 checks passed
@DmitrySharabin DmitrySharabin deleted the color-info-prop branch October 16, 2024 10:38
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