[color-picker] If no color is provided, use the default one

[DmitrySharabin]

This PR also fixes this Codepen: https://codepen.io/leaverou/pen/wvbzovK

Without this fix, the <color-picker> may appear to be functioning correctly at first glance. However, this is due to the default color of the underlying <color-slider>s, which happens to match the picker's default color. The issue lies in the fact that the picker's default color was not being propagated to the underlying elements. This can be observed by looking at the swatch or by changing the defaultColor prop (by setting its default property to red) and noting that the change does not take effect.

[netlify[bot]]

✅ Deploy Preview for color-elements ready!

Name	Link
🔨 Latest commit	30691d1d03b2d167fc74ff1dd892bbafcddf06ba
🔍 Latest deploy log	https://app.netlify.com/sites/color-elements/deploys/6669b5884b6ba70008e6bb75
😎 Deploy Preview	https://deploy-preview-91--color-elements.netlify.app
📱 Preview on mobile	Toggle 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]

Hmmm not sure I like setting color when it's just the default. Then we lose track of whether this has been user set or just the default value. What if we instead change the condition to name === "color" || name === "defaultColor"?

This works, too. One thing that stopped me (when I tried it yesterday) from using this approach is that the colorchange event is not fired after the component is mounted in that case. This is expected from my perspective, but is this what the users expect?

[LeaVerou]

Hmmm not sure I like setting color when it's just the default. Then we lose track of whether this has been user set or just the default value. What if we instead change the condition to name === "color" || name === "defaultColor"?

This works, too. One thing that stopped me (when I tried it yesterday) from using this approach is that the colorchange event is not fired after the component is mounted in that case. This is expected from my perspective, but is this what the users expect?

That sounds like a separate bug. If color has no value and its default value changes, propchange (and thus colorchange) should fire. Instead of working around our own bugs, we should be fixing them!

[DmitrySharabin]

That sounds like a separate bug. If color has no value and its default value changes, propchange (and thus colorchange) should fire. Instead of working around our own bugs, we should be fixing them!

this.color should be equal to this.defaultColor anyway if it has no value. Is it not?

OK, here is the current state after I took another step toward solidifying my knowledge of NudeElement and color elements props.

For now, defaultColor is not the default prop for color: https://github.com/color-js/elements/blob/13a7898e371975237b497192dd6eea0b3725bfc7/src/color-picker/color-picker.js#L131-L136

That's why we might stumble upon a situation when this.color is undefined. Until we implement https://github.com/nudeui/element/issues/24, we can't simply use defaultProp: "defaultColor" for color; we have to use a getter and setter instead (see https://github.com/color-js/elements/issues/85#issuecomment-2149710030 as an example of why it happens).

With this PR, we fix the bugs mentioned above by Lea instead of working around them.

Another thing is that when we set space (whether specified or not, but especially if it wasn't), we should update the color of underlying sliders and not build a new color based on the sliders' current values. With this change, the color picker from docs and the one from Color.js work identically (for cases when alpha is equal to 1). And this is definitely a good thing! 🙂

So, let's get another iteration on this started.

[LeaVerou]

Pretty sure there was a reason they were separate though. 🤔

[LeaVerou]

I think one of them was meant to represent the color provided in the HTML (or the color space midpoint, if none was provided), and the other one the current color. I.e. defaultColor was not meant to change.

[DmitrySharabin]

I think one of them was meant to represent the color provided in the HTML (or the color space midpoint, if none was provided), and the other one the current color. I.e. defaultColor was not meant to change.

Makes sense!

It means that we could have something like this, right?

defaultColor: {
    type: Color,
    convert (color) { },
    default () { },
    reflect: {
        from: "color",
    },
},

color: {
    type: Color,
    defaultProp: "defaultColor",
    reflect: false,
},

We might want to wait for changes on Nude Element to make everything right or use changes in this PR as a workaround and fix everything properly after Nude Element is ready. What do you think?

[LeaVerou]

I think one of them was meant to represent the color provided in the HTML (or the color space midpoint, if none was provided), and the other one the current color. I.e. defaultColor was not meant to change.

Makes sense!

It means that we could have something like this, right?

defaultColor: {
  type: Color,
  convert (color) { },
  default () { },
  reflect: {
      from: "color",
  },
},

color: {
  type: Color,
  defaultProp: "defaultColor",
  reflect: false,
},

I belive so, yes.

We might want to wait for changes on Nude Element to make everything right or use changes in this PR as a workaround and fix everything properly after Nude Element is ready. What do you think?

What changes? This is not the same as rawProp/parsedProp.

[DmitrySharabin]

What changes? This is not the same as rawProp/parsedProp.

Right, it has nothing to do with the issue I'm trying to solve. 🤦‍♂️

Splitting color and defaultColor actually works really fine. What was I thinking of earlier? It's such a straightforward thing to try. 🤷‍♂️