Some fixes #44
Some fixes #44
Conversation
but only if the space is changed for real
Eventually, everything will be settled down.
✅ Deploy Preview for color-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
`progress` depends not only on the current value but also on the range this value belongs to.
src/channel-slider/channel-slider.js
Outdated
| return this.defaultColor.get(this.channel); | ||
| } | ||
| catch { | ||
| let channel = Object.keys(this.space.coords)[0]; |
There was a problem hiding this comment.
A comment here about what it means to be in this branch?
There was a problem hiding this comment.
Every time we set space on <color-picker>, we update space and channel of underlying <channel-slider>s. This leads to updates defaultColor and, eventually, defaultValue. And this line produced many errors from Color.js when we tried to get no existing channel from the color.
After neutralizing these errors, I could find why the dynamic set of space didn't work.
| @@ -80,6 +80,10 @@ const Self = class ColorPicker extends NudeElement { | |||
| this._el.sliders.insertAdjacentHTML("beforeend", `<channel-slider space="${ this.space.id }" channel="${ channel }"></channel-slider>`); | |||
| } | |||
| } | |||
|
|
|||
| if (change.oldInternalValue || change.oldAttributeValue) { | |||
| this.render(); | |||
There was a problem hiding this comment.
Why? A comment would be helpful here!
There was a problem hiding this comment.
When we first come here on component initialization, there is no need to call render() since it'll be rendered automatically after it's connected. So, we only need to re-render the component when space actually changes after the component is mounted.
There were no issues when I called render() without any condition, but I thought I could optimize it a bit by not re-rendering the component when there is no need.
src/color-slider/color-slider.js
Outdated
| @@ -99,10 +99,10 @@ const Self = class ColorSlider extends NudeElement { | |||
| this.style.setProperty("--color", displayedColor); | |||
| } | |||
| } | |||
| else if (name === "value") { | |||
| else if (name === "value" || name === "min" || name === "max") { | |||
There was a problem hiding this comment.
This could also be a direct commit (together with the line below)
There was a problem hiding this comment.
Let me commit this change and the one above (with a fix for the typo) directly and rebase this branch so we can iterate.
src/channel-slider/channel-slider.js
Outdated
| } | ||
| catch { | ||
| let channel = Object.keys(this.space.coords)[0]; | ||
| return this.defaultColor.get(channel); |
There was a problem hiding this comment.
If we're just trying to get the first coord, it’s easier to do this.defaultColor.coords[0].
There was a problem hiding this comment.
Oh, yes. Much easier. Thanks!
|
Close this issue since I broke it into two separate PRs: |
One thing is left: after updating
space,progressis not updated. Investigating...That's why I submit it as a draft.