search

NicolasCARPi / jquery_jeditable

jQuery edit in place plugin. Extendable via plugin architecture. Plugins for plugin. Really.
https://jeditable.elabftw.net
MIT License
1.74k stars 458 forks source link

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

Closed eman1986 closed 6 years ago

eman1986 commented 6 years ago

causing duplicate entries

Please make sure to target the "experimental" branch!

NicolasCARPi commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Published on npm!