Skip to content

utilize classList when supported (refs #5087) #1190

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from

Conversation

mgol
Copy link
Member

@mgol mgol commented Mar 2, 2013

No description provided.

@rwaldron
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
Member Author

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.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants