[color-picker] Fix regression when provided color was replaced by the default one on component initialization

DmitrySharabin commented 6 months ago

When we render <color-picker> in the process of its initialization (on space change), we replace the color (provided via the color attribute) with the default one. We definitely don't want it. I noticed this earlier, but yesterday, it slipped my mind. Sorry about that. This PR should fix the regression.

netlify[bot] commented 6 months ago

Deploy Preview for color-elements ready!

Name	Link
Latest commit	83e01d8813ad6401e9f5dbf37a98508dca3b87e8
Latest deploy log	https://app.netlify.com/sites/color-elements/deploys/664c69cfaa559d00087e96ec
Deploy Preview	https://deploy-preview-51--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.

LeaVerou commented 6 months ago

This sounds a bit like a workaround rather than a fix at first glance. render() should not be replacing the current color with the default one, that sounds like a bug in render(). Could you tell me a bit more about what happens?

DmitrySharabin commented 6 months ago

Sure.

On component initialization, space is set, and render() is called, the following block is executed:

https://github.com/color-js/elements/blob/e8ec6d801de8b31dc8bf3a9cda34a13ef1817d6d/src/color-picker/color-picker.js#L48-L51

In this block, a new color is being built based on the channel sliders' values (line 48), i.e., the default ones. Then, it is set in line 49. The corresponding setter updates the color attribute, and we lose the provided color before it was read and applied.

Hope it makes sense.

LeaVerou commented 6 months ago

This whole render method feels like a relic; we shouldn't need it, prop defaults and propChangeCallback() should take care of these things in a better way.

DmitrySharabin commented 6 months ago

This whole render method feels like a relic; we shouldn't need it, prop defaults and propChangedCallback() should take care of these things in a better way.

Agreed.

Thinking about this issue a bit more, I recalled that we face this issue (and similar issues in other components) because propChangedCallback() might be called even though the component hasn't been fully initialized (connected) yet.

For example, when we set min, we should also set it on the spinner. However, the value that min should have on the spinner depends on the value of tooltip: if this.tooltip === "progress", min should always be equal to 0 regardless. Unfortunately, this.tooltip might be undefined if the corresponding attribute (prop) hasn't been handled by the moment min is set.

I wonder if it is possible to queue all calls of propChangedCallback() (while the component is being initialized) and perform them one by one after the component is connected (when we have all the information we need). Some of those calls probably cancel the previous ones, but it's not a big deal (at least for now).

LeaVerou commented 6 months ago

That is an excellent point — can you file an issue on Nude Element?

DmitrySharabin commented 5 months ago

I'm closing this one in favor of #87. There, I implemented ideas mentioned in this PR, e.g., getting rid of the relic render().

color-js / elements