Unselect events: cache lookup key fix #6179
Conversation
…ing the original 'option' element as the cache lookup key instead of the event target
…on -- not selection -- events
… to filter relevant 'option' elements
| @@ -114,7 +114,12 @@ define([ | |||
| }); | |||
|
|
|||
| container.on('unselect', function (params) { | |||
| self.unselect(params.data); | |||
| container.$element.find('option').each(function () { | |||
| if ($(this).val() == params.data.id) { | |||
There was a problem hiding this comment.
Note: this is somewhat intentionally not a complete-strict equality test (in other words: it is expected that this is == instead of ===)
This follows a similar equality pattern check in array.js here.
If both option-value and data-id are guaranteed to be of type string, then this could be upgraded to a strict equality test.
| @@ -114,7 +114,12 @@ define([ | |||
| }); | |||
|
|
|||
| container.on('unselect', function (params) { | |||
| self.unselect(params.data); | |||
| container.$element.find('option').each(function () { | |||
There was a problem hiding this comment.
@kevin-brown I'm feeling a bit uncertain about whether this change is a good idea, after looking at it again.
This extra for loop feels like it could quite easily introduce performance regressions.
I'll find time to take another look at this soon, but if in any doubt, please feel free to revert this change.
There was a problem hiding this comment.
(update: re-inspecting this changeset currently)
There was a problem hiding this comment.
Yep, a slightly stronger statement after re-reading the changeset and related issues: I think I'd prefer for this pull request to be reverted (and #6128 re-opened), if that's OK.
I didn't add any test coverage as part of the changes, which was unwise, because that makes it much harder to determine whether the modifications truly resolve the issue identified in the linked ticket.
There was a problem hiding this comment.
(revert pull request opened as #6217)
…select2#6217)" This reverts commit 0a186eb.
…2#6179)" (select2#6217)"" This reverts commit c80c331.
* During selection and unselection events, retrieve cached item data using the original 'option' element as the cache lookup key instead of the event target * Fixup: only use 'option' element as cache lookup key during unselection -- not selection -- events * Narrow down 'option' element query to find the unselected item by value * Consistency: prefer jQuery.find...each pattern as used elsewhere in the codebase * Lint fixup: fit within line length limits * Use equality checks instead of jQuery/CSS selector attribute matching to filter relevant 'option' elements
* During selection and unselection events, retrieve cached item data using the original 'option' element as the cache lookup key instead of the event target * Fixup: only use 'option' element as cache lookup key during unselection -- not selection -- events * Narrow down 'option' element query to find the unselected item by value * Consistency: prefer jQuery.find...each pattern as used elsewhere in the codebase * Lint fixup: fit within line length limits * Use equality checks instead of jQuery/CSS selector attribute matching to filter relevant 'option' elements
This pull request includes a
The following changes were made
unselectevents, retrieve the HTMLoptionelement related to the unselection and use that as a cache key to retrieve item dataIf this is related to an existing ticket, include a link to it as well.
Resolves #6128.