color-js / elements

WIP
12 stars 1 forks source link

[color-swatch] Regression: If `color` is set programmatically, `value` doesn't update #85

Closed DmitrySharabin closed 3 months ago

DmitrySharabin commented 3 months ago

Testcase

<color-swatch id="dynamic_static">oklch(70% 0.25 138)</color-swatch>
<button onclick='dynamic_static.color = "oklch(60% 0.15 0)"'>Change color</button>
DmitrySharabin commented 3 months ago

One more thing to note. If <color-swatch> is editable after the color is set programmatically, changing the value in <input> no longer updates the color. The link between value and color is broken after the color change.

DmitrySharabin commented 3 months ago

The link between value and color is broken after the color change.

OK, this is why it happens (at least, as far as I can see).

value is the default prop for color: https://github.com/color-js/elements/blob/92cb2e0f8a7beb09eb4f23f807ff2f03b58e82c9/src/color-swatch/color-swatch.js#L134 Every time we update value, color, as a dependent of value, is also updated. This happens because dependsOn() returns true.

But why does it return true in our case? Because of this line:

this.defaultProp === prop && element.props[this.name] === undefined

value is the color default prop and element.props["color"] === undefined (since we haven't set it programmatically yet).

When we set color programmatically, we set the corresponding prop on the element. This means element.props["color"] is not undefined anymore. In its turn, it means that dependsOn() will return false when the value prop updates, and the dependent (color) won't be updated—changes to value won't be propagated to color.

I wonder, what should be done in this situation?

DmitrySharabin commented 3 months ago

value should not be the default prop for color and color, in its turn, should get its default value the other way?

I tried this. If we replace defaultProp with a getter and a setter (and one tiny change in propChangedCallback()), the issue with editable swatches will be fixed. It also fixes the issue with <color-picker> in one shot. It looks like we don't need parse() in this case.

color: {
    type: Color,
    // defaultProp: "value",
    get () {
        if (!this.value) {
            return null;
        }

        return Color.get(this.value);
    },
    set (value) {
        this.value = Color.get(value)?.display();
    },
    // parse (value) {
    //  if (!value) {
    //      return null;
    //  }

    //  return Color.get(value);
    // },
    reflect: false,
},
LeaVerou commented 3 months ago

Another solution is to implement https://github.com/nudeui/element/issues/24 which would handle keeping them in sync automatically.

DmitrySharabin commented 3 months ago

For now, we decided to go with a getter and setter (as a workaround) to make the stuff work again.