Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug] sl-color-chooser does not honor the value when it is set from javascript #1141

Closed
hyyan opened this issue Jan 22, 2023 · 5 comments
Closed
Assignees
Labels
bug Things that aren't working right in the library.

Comments

@hyyan
Copy link

hyyan commented Jan 22, 2023

In the attached sample. The value of the color picker is not honored when the component is initialized . Interestingly, if I set the value as an attribute then it is honored.

<link rel="stylesheet" href="https://cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/themes/light.css" />
<script type="module" src="https://cdn.jsdelivr.net/npm/@shoelace-style/shoelace@2.0.0-beta.88/dist/shoelace.js"></script>

<!-- Works -->
<sl-color-picker label="Select a color" value="#000"></sl-color-picker>
<!-- Does not work -->
<sl-color-picker id="c2" label="Select a color"></sl-color-picker>

<script>
  const c = document.querySelector("#c2")
  c.value='#000';
</script>
@hyyan hyyan added the bug Things that aren't working right in the library. label Jan 22, 2023
@xdev1
Copy link
Contributor

xdev1 commented Jan 22, 2023

This is a bug in Shoelace version 2.0.0-beta.88, but due to the recent changes in file color-picker.ts this should not be a problem any longer.
Nothing to do here (at least if my evaluation was correct) ... just waiting for the next beta release.

[Edit - a few hours later] The previous sentence is wrong! In my tests I've used colorPicker.value = '#ff0000' (= red) and everything seemed to work fine without any changes. If I had used a different color than red, then I would have seen that things are not working properly.

@claviska
Copy link
Member

When set early enough, the initial value was being overridden on connectedCallback(). Moving this logic to firstUpdated() solves it and prevents the component from unnecessarily updating again when it gets moved around in the DOM.

Fixed in 93158e8. This will be available in the next release.

@xdev1
Copy link
Contributor

xdev1 commented Jan 22, 2023

@claviska Please allow me to mention that using firstUpdated here is really quite confusing. Whenever firstUpdated was used in Shoelace in the past it was always for a good reason: Direct DOM manipulation, validity stuff etc.
The latest change in color-picker.ts is the first time in SL where firstUpdated is used in a quite unexpected way (it feels not really necessary and the component will be initially rendered twice in some cases, which is very unusual). As the handleValueChange method will always be called after the color picker is connected (because value has an initial value of '' which differs from undefined), wouldn't it be easier just to remove the whole firstUpdated method completely and modify method handleValueChange a bit (I don't know if this does the job completely, but at least it seemed to work in my tests)?

image

claviska added a commit that referenced this issue Jan 23, 2023
claviska added a commit that referenced this issue Jan 23, 2023
@claviska
Copy link
Member

This wasn't sitting right with me, either. I dug deeper and came to the same conclusion — this isn't required anymore. I cleaned that up, remove the no longer used lastValueEmitted property, and added a call to this.syncValues() so the initial value is normalized.

@claviska
Copy link
Member

See 5cdbaa8 and 44ecc8c.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Things that aren't working right in the library.
Projects
None yet
Development

No branches or pull requests

3 participants