-
Notifications
You must be signed in to change notification settings - Fork 610
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
Fix #573 #574
Fix #573 #574
Conversation
Thanks for this - i've left some comments 👍 |
Co-Authored-By: Josh Johnson <[email protected]>
Co-Authored-By: Josh Johnson <[email protected]>
Co-Authored-By: Josh Johnson <[email protected]>
Co-Authored-By: Josh Johnson <[email protected]>
Co-Authored-By: Josh Johnson <[email protected]>
Tested this, and can confirm that it works in my app. Great job! |
- Save `passedElement.options` values as `this._presetOptions` - Restore saved `this._presetOptions` to `passedElement.options` on `destroy` - It avoids restoring options in `this.config.choices`
@jshjohnson: Maybe you can comment on the current status of this? |
I'd love to see this resolved. Anything I can do to help? |
@jshjohnson re-review please? |
Co-Authored-By: Josh Johnson <[email protected]>
Co-Authored-By: Josh Johnson <[email protected]>
Co-Authored-By: Josh Johnson <[email protected]>
@jshjohnson Then, I have gotten a new ambiguity. <select>
<option selected>one</option>
<option>two</option>
<option>three</option>
</select>
<script>
const select = document.querySelector('select')
console.log(select.value) // => one
const choices = new Choices(select)
// select "two" on UI
choices.destroy()
console.log(select.value) // what?
</script> How do you think it should work? Thanks! |
For a first pass I would say no. We can revisit this is people want to though. P.s. you have some conflicts! |
Thanks! I got it.
I have fixed. |
@jshjohnson Can't wait for this to be released! Thanks! |
src/scripts/choices.js
Outdated
@@ -144,6 +144,7 @@ class Choices { | |||
classNames: this.config.classNames, | |||
template: data => this._templates.option(data), | |||
}); | |||
this._presetOptions = this.passedElement.options; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this be moved above line 186 please? That's where all the "preset" variables are defined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I have fixed.
@kzkn Thank you, Sir! @jshjohnson Thank you, as well! Is there any chance to get this updated version on NPM? :) |
@jshjohnson Ace, thanks! |
@jshjohnson Sorry for bugging you about this but I think there is an issue with the destroy() method. It doesn't seem to pass the selected value? I'm on 9.0.1 and let's say, the select I initialize Choices on has the selected value of "3". I destroy that Choices object and, unfortunately, the selected value on the tag is not "3". It gets "reset" I believe. |
That's expected @filip-jk #574 (comment) |
@jshjohnson I'm confused. I'm initializing Choices on a select that has a selected value of "5". Choices take that selected value and I can see it in the rendered Choices dropdown menu. Once I destroy it, the select gets totally reset so it's not really the states the Choices was initialized on, right? If that's expected than the description of the method is wrong: "Kills the instance of Choices, removes all event listeners and returns passed input to its initial state." It's not the initial state if the selected option/value is different. |
Yeah you're right, I would expect the originally selected option to persist. I'll investigate |
Hi, is it possible to reopen this issue? I'm experiencing the same issue described by @filip-jk The use case is in a stimulus.js + turbo application, I will try to provide a reduced test case, but I'm going to describe it
|
Description
This PR fixes #573. It contains following fixes:
destroy
init
How Has This Been Tested?
E2E tests are added.