Ensure choice count is reflective of actual selected item count. #1171
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
I ran into some issues where updated multi selects from external javascript would frequently get out of state because the .choices variable on the Chosen object was no longer the same as the actual number of options selected. Instead of having a global variable on the chosen jquery object (which is relatively fragile because of external changes to the select element), just calculate it as needed. It's not called often and this is a fairly fast lookup for jquery and prototype, so I think this is a safe change.
Conflicts: chosen/chosen.jquery.js chosen/chosen.jquery.min.js chosen/chosen.proto.js chosen/chosen.proto.min.js coffee/chosen.jquery.coffee coffee/chosen.proto.coffee
Testing in Chrome showed this to be anywhere from a 5 to 12 time speed improvement. It also lets us have one method in abstract instead of 2.
|
Thanks for looking into this, I'll see if it fixes the issue we ran into. |
|
Looks good to me if performance does not suffer (I haven't tried both versions to compare them) |
gets selected in the form field. This fixes #977
|
@stof @kenearley I've added a copule more commits to this diff. The first adds some memorization of the choices count -- improving performance overall while removing the likely trouble spots that lead me to make these changes in the first place. The second fixes an issue with the |
|
this looks good to me |
|
👍 |
Conflicts: chosen/chosen.jquery.js chosen/chosen.jquery.min.js chosen/chosen.proto.js chosen/chosen.proto.min.js coffee/chosen.jquery.coffee coffee/chosen.proto.coffee coffee/lib/abstract-chosen.coffee
|
Awesome! 👍 |
pfiller
added a commit
that referenced
this pull request
May 23, 2013
Ensure choice count is reflective of actual selected item count.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Original pull request is #1070 by @tjarratt
This addresses some apparent issues where the
@choicesvariable can get out of sync with the actual selected options length. It simply returns the number of selected elements on the underlying select box.@kenearley @stof What do you think of this? I haven't been able to recreate the situation described by @tjarratt, but I don't really have a problem with this. Maintaining a variable with little performance gain seems unnecessary anyways.
I've done some click testing and it seems performant (especially so in modern browsers). In IE8 with 2k+ options, it's slow, but not noticeably slower than master.