Skip to content
This repository was archived by the owner on Jul 29, 2022. It is now read-only.

Fixed a bug where selected value will get added to the select list,#166

Merged
NicolasCARPi merged 1 commit intoNicolasCARPi:masterfrom
eman1986:master
Mar 28, 2018
Merged

Fixed a bug where selected value will get added to the select list,#166
NicolasCARPi merged 1 commit intoNicolasCARPi:masterfrom
eman1986:master

Conversation

@eman1986
Copy link
Contributor

causing duplicate entries

Please make sure to target the "experimental" branch!

@NicolasCARPi
Copy link
Owner

NicolasCARPi commented Mar 28, 2018

Hello,

Thank you for your contribution. But:

When I write "Please make sure to target the "experimental" branch!" it means you need to target the experimental branch, not the master one!

Also, can you elaborate a bit on what you are trying to fix? Why do you think it's a good idea to remove the selected value from the list? How is that a bug? What if the user changes his mind and want to reselect the selected value again to avoid change?

Please provide a jsfiddle to test this, or at least steps to reproduce on the demo.

Finally, your code is not optimal because you are declaring the "option" variable in an "if". If you use a linter, you'll see that it complains.

@eman1986
Copy link
Contributor Author

I'm sorry about putting it in the wrong branch, I can fix it so that its in the right branch.

regarding whats broken? consider the following value:

'{"0":"No","1":"Yes","selected":"Yes"}'

this will output as:

-> Yes
-> Yes
-> No

my fix will output it as:

-> Yes
-> No

Selected is duplicate value, if the user changes their mind they can just cancel right? this is isn't saving them from reverting from their original choice, just duplicating the selected value, which my customers have thrown a fit about when they discovered it.

I would provide a jsfiddle but seeing as you have no cdn for the latest version, this is all but impossible unless you expect me to use my bandwidth or I don't know of a special version available.

@NicolasCARPi
Copy link
Owner

Ok I see. I remember fiddling with this when merging the old PRs. It is very likely that the old behaviour was what you describe where you add another entry for the selected item (in fact this is what the doc suggests!). It makes more sense to do it like this anyway, because otherwise you lose the key of the selected one.

As for the jsfiddle, you could use this for for loading the plugin: https://jeditable.elabftw.net/src/jquery.jeditable.js but no need for that now because you've explained better so I could reproduce.

I'll merge and push to npm asap :)

Cheers,
~Nico

@NicolasCARPi NicolasCARPi merged commit 1db656c into NicolasCARPi:master Mar 28, 2018
NicolasCARPi added a commit that referenced this pull request Mar 28, 2018
@NicolasCARPi
Copy link
Owner

Published on npm!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants