-
Notifications
You must be signed in to change notification settings - Fork 5.3k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Widget: Increase performance for large sortable/draggable collections #2313
base: main
Are you sure you want to change the base?
Conversation
@@ -355,7 +355,7 @@ $.Widget.prototype = { | |||
|
|||
this._destroy(); | |||
$.each( this.classesElementLookup, function( key, value ) { | |||
that._removeClass( value, key ); | |||
that._removeClass( $( Array.from( value ) ), key ); |
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.
I'm not that worried about this since it's in destroy
. That shouldn't happen too often.
this.bindings.off( this.eventNamespace ); | ||
$( Array.from( this.bindings ) ).off( this.eventNamespace ); |
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.
I'm not that worried about this since it's in destroy
. That shouldn't happen too often.
// below. | ||
elements = $( currentElements.get() ); | ||
this._removeClass( currentElements, classKey ); | ||
elements = $( Array.from( currentElements ) ); |
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.
We re-wrap here, but the previous code was already creating a new jQuery object, so it shouldn't be worse than before.
Also, I don't understand the comment above; how would we lose the reference here? It's already in the currentElement
object, that wouldn't just disappear...
c2e2519
to
67457c4
Compare
Fixes gh-2062