Consider removing or patching toggleClass() #3338

Closed
peterflynn opened this Issue Apr 4, 2013 · 4 comments

Comments

Projects
None yet
3 participants
@peterflynn
Member

peterflynn commented Apr 4, 2013

jQuery's toggleClass() is an easy method to misuse: the 2nd argument is a boolean, but it won't work as intended unless it's literally true or false. Truthy/falsy values are ignored.

Some of our code explicitly acknowledges this via Boolean() casts, or by using an if with add/RemoveClass() instead (e.g. Menu.js). But lots of our code simply skates by because it happens to use !. And in at least one place -- ViewUtils -- we don't even have that assurance. (See #3184 for another case of how easy it is to get snared by this).

From what I understand, jQuery does this to remain compatible with the jQuery-UI-patched version of toggleClass() where the 2nd argument could also be a number that means something totally different (animation). So there's little chance of a "fix" in future jQuery versions.

So to reduce brittleness, it seems like we should either:
a) Have a policy of not using toggleClass() anywhere. Use the more verbose add/removeClass() form instead.
b) Have a policy of not using toggleClass() anywhere. Create our own toggleClass() utility function that we use instead.
c) Create a little jQuery plugin that patches the standard jQuery.toggleClass() to behave more safely.

Personally I lean toward C since it doesn't require constant enforcement... but the downside is if we ever start using jQuery UI later, then this API would behave differently than the jQ UI docs specify.

@redmunds

This comment has been minimized.

Show comment
Hide comment
@redmunds

redmunds Apr 8, 2013

Contributor

I wasn't aware of that behavior, but I think it's bad enough that we should fix it in jQuery and submit a pull request.

I think toggleClass() is only appropriate in only a very few, simple cases, so we can convert to add/removeClass() in most places and the few remaining cases can be fixed in place (with a comment) until jQuery is fixed.

Contributor

redmunds commented Apr 8, 2013

I wasn't aware of that behavior, but I think it's bad enough that we should fix it in jQuery and submit a pull request.

I think toggleClass() is only appropriate in only a very few, simple cases, so we can convert to add/removeClass() in most places and the few remaining cases can be fixed in place (with a comment) until jQuery is fixed.

@pthiess

This comment has been minimized.

Show comment
Hide comment
@pthiess

pthiess Apr 8, 2013

Member

Reviewed

Member

pthiess commented Apr 8, 2013

Reviewed

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn May 1, 2013

Member

@redmunds We'd never be able to merge a patch upstream -- that idea was rejected a while back because it would break backwards-compatibility with the way jQuery UI grafts on animation capabilities. (IMHO, this is a perfect example of the downside to jQuery's "endless overloading" API design... but that architectural choice was set in stone long ago).

Member

peterflynn commented May 1, 2013

@redmunds We'd never be able to merge a patch upstream -- that idea was rejected a while back because it would break backwards-compatibility with the way jQuery UI grafts on animation capabilities. (IMHO, this is a perfect example of the downside to jQuery's "endless overloading" API design... but that architectural choice was set in stone long ago).

@peterflynn

This comment has been minimized.

Show comment
Hide comment
@peterflynn

peterflynn May 14, 2013

Member

Pull request from @WebsiteDeveloper has landed, following approach B (except in a few places where it was cleaner to go with A). I've updated the coding style guide to mention that we should not use $.toggleClass(). Feels good enough to close for me.

Member

peterflynn commented May 14, 2013

Pull request from @WebsiteDeveloper has landed, following approach B (except in a few places where it was cleaner to go with A). I've updated the coding style guide to mention that we should not use $.toggleClass(). Feels good enough to close for me.

@peterflynn peterflynn closed this May 14, 2013

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