color-js / elements

WIP
13 stars 1 forks source link

[color-chart] Doesn't work properly when no labels are provided #103

Closed LeaVerou closed 2 weeks ago

LeaVerou commented 4 months ago

We resolved that it should default to the color index, but it seems to be incorrect.

image
DmitrySharabin commented 4 months ago

When we use <color-scale> as a source of colors to be plotted, there is no way that colors won't get a label—by default, the color itself will become the label:

https://github.com/color-js/elements/blob/805459eb4db244f7a9169e28bcc9eccb94f72b0e/src/color-scale/color-scale.js#L111

I wonder if there is a way to get the raw string used when building the corresponding color. If so, we could compare that string with the generated label, and if they are equal, the color index should be used as an X-coordinate. 🤔

DmitrySharabin commented 4 months ago

I can see that every color has parseMeta. Let's see if it can help.

LeaVerou commented 4 months ago

This is quite hacky. Let me think how we can solve this properly.

LeaVerou commented 4 months ago

I think we need to have a distinct concept of a color name/label, rather than "sometimes it's the element content and sometimes it's not, sometimes it's the color itself, sometimes it's not". The color itself should be shown at all times, and if authors wish not to show it, they can always hide the corresponding part.

I also don't like this weird API where we slot an input to indicate the element is editable. We should have a separate attribute for that that generates the input, and we should be able to optionally slot a custom input if we want to apply custom things to it or have it participate in form submission (rather than recreating the whole input API on <color-swatch>).

LeaVerou commented 3 weeks ago

What if we could set a defaultLabel prop (no attribute) to a function so that using components could override? It would take the color and index as an arg

DmitrySharabin commented 3 weeks ago

What if we could set a defaultLabel prop (no attribute) to a function so that using components could override? It would take the color and index as an arg

Could you elaborate on how you see it will be used, please?

LeaVerou commented 3 weeks ago

Could you elaborate on how you see it will be used, please?

By default it would be set to (c, i) => i (or (c, i) => i + 1 so that it starts from 1) but people could do myColorChart.defaultLabel = customFunction to set it to whatever. For example, to use OKLCH Lightness instead, rounded to the nearest 5, people could use myColorChart.defaultLabel = c => Math.round(c.oklch.l/5) * 5.

Or perhaps instead of defaultLabel we should use getX, since we're framing everything else around the Y axis (and that also hints that the result should be a number).

DmitrySharabin commented 2 weeks ago

Could you elaborate on how you see it will be used, please?

By default it would be set to (c, i) => i (or (c, i) => i + 1 so that it starts from 1) but people could do myColorChart.defaultLabel = customFunction to set it to whatever. For example, to use OKLCH Lightness instead, rounded to the nearest 5, people could use myColorChart.defaultLabel = c => Math.round(c.oklch.l/5) * 5.

Or perhaps instead of defaultLabel we should use getX, since we're framing everything else around the Y axis (and that also hints that the result should be a number).

Thank you for the explanation. Does this mean that to use X-coord from the color label, the author should define a custom getX() function? I.e., <color-chart> won't try to take them from the color label? So, this

image

and this

image

will become impossible by default?

What am I missing?

LeaVerou commented 2 weeks ago

Ah, good point, getX() should take the label as a parameter as well, if one exists, and the default value should encapsulate the current logic for extracing that.

DmitrySharabin commented 2 weeks ago

I managed to find a fix for this bug without introducing getX().

However, I find the concept of the getX() prop very useful, but I suggest we move the discussion around it to a separate issue. What do you think?

LeaVerou commented 2 weeks ago

I'm really not sure about that fix, I think it would break a lot of legit usage, and would make it very cumbersome. Agreed we need a separate issue for getX().