utilize classList when supported (refs #5087) #1190

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
2 participants
@mgol
Member

mgol commented Mar 2, 2013

No description provided.

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Mar 2, 2013

Member

Thanks for the contribution! As owner of http://bugs.jquery.com/ticket/5087 I can assure you I haven't forgotten! The classList API is still a poor replacement, performance wise, when dealing with multiple classes per call.

Member

rwaldron commented Mar 2, 2013

Thanks for the contribution! As owner of http://bugs.jquery.com/ticket/5087 I can assure you I haven't forgotten! The classList API is still a poor replacement, performance wise, when dealing with multiple classes per call.

@rwaldron rwaldron closed this Mar 2, 2013

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 2, 2013

Member

Look at my last comment under http://bugs.jquery.com/ticket/5087, I created a few test cases which show we get a large performance boost after applying this patch... What am I missing?

Member

mgol commented Mar 2, 2013

Look at my last comment under http://bugs.jquery.com/ticket/5087, I created a few test cases which show we get a large performance boost after applying this patch... What am I missing?

@rwaldron

This comment has been minimized.

Show comment
Hide comment
@rwaldron

rwaldron Mar 2, 2013

Member

That comment wasn't there when I visited the ticket just before closing the PR... I've owned the ticket and babysit the implementation schedule for years and I stand by these decisions:

  1. There is no benefit to adding more code that does the same thing that jQuery already does
  2. Any performance benefits are outweighed by 1
  3. Multiple classes have been a performance issue
  4. jQuery will not use classList until it accepts multiple class args to the add and remove APIs, at which time we'll remove the old way and completely replace it.
Member

rwaldron commented Mar 2, 2013

That comment wasn't there when I visited the ticket just before closing the PR... I've owned the ticket and babysit the implementation schedule for years and I stand by these decisions:

  1. There is no benefit to adding more code that does the same thing that jQuery already does
  2. Any performance benefits are outweighed by 1
  3. Multiple classes have been a performance issue
  4. jQuery will not use classList until it accepts multiple class args to the add and remove APIs, at which time we'll remove the old way and completely replace it.
@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol Mar 2, 2013

Member

Well, multiple classes don't seem to be a performance issue anymore, at least it didn't seem so in my test cases on jsFiddle, even with the current limited classList API.

But I understand that as the className path is required even for jQuery 2.0 because of IE9, your first two points have settled it by themselves so I respect your decision.

Member

mgol commented Mar 2, 2013

Well, multiple classes don't seem to be a performance issue anymore, at least it didn't seem so in my test cases on jsFiddle, even with the current limited classList API.

But I understand that as the className path is required even for jQuery 2.0 because of IE9, your first two points have settled it by themselves so I respect your decision.

@ameyms ameyms referenced this pull request Oct 24, 2013

Closed

Improve jQuery classes.js #1403

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