Skip to content
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

Toggle class #47

Closed
wants to merge 4 commits into from
Closed

Conversation

simeydotme
Copy link
Collaborator

Updated the hasClass method and added a toggleClass method, both now will work on the entire collection and accept multiple classes.

hasClass will return false if any of the items in the collection do not have the given class(es); - to revert to the original behaviour it's as simple as $("el").first().hasClass("className");

Hope you like it, I htink it's a good improvement on the default functionality and another 1up on jQuery?

Creating in response to #46

@shshaw shshaw mentioned this pull request Mar 1, 2016
@gamebox
Copy link

gamebox commented Apr 9, 2016

It would be interesting to know what's the status with this. It's a tad confusing to have toggleClass in the docs, but not actually in the codebase. Something I can do to help?

@simeydotme
Copy link
Collaborator Author

Seems this method was copied/converted in a large merge with another library in #66 ... Not sure if that was released yet... The readme unfortunately always takes the master trunk, and not the latest release... So it's usually ahead of the stable release (hence the method is in the docs, but not the releases code)

@gamebox
Copy link

gamebox commented Apr 9, 2016

Ah, thanks @simeydotme . So I would suggest closing this, and discuss how to keep releases and docs in sync.

@shshaw shshaw mentioned this pull request Apr 12, 2016
@shshaw
Copy link
Collaborator

shshaw commented Apr 12, 2016

Both of these fixes were implemented in different ways as of version 1.1.0. Thanks!

@shshaw shshaw closed this Apr 12, 2016
@simeydotme
Copy link
Collaborator Author

simeydotme commented Apr 18, 2016

@shshaw ... did you implement my .hasClass() method with the same functionality? it seems not because I'm just writing unit tests now based on my previous set; and it's failing for:

<i class="a b x y  "></i>
<i class="a b x    "></i>
<i class="a b   y z"></i>

$("i").hasClass("x y z"); // errors, because checking for multiple classes;

It doesn't handle multiple classes, just crashes. I think it should make sure all classes are present as was the case with my PR... and is currently the case where .toggleClass() will toggle all the classes? 😁

I know that actually my PR was never really reviewed/accepted by @kenwheeler , and so I'm willing to accept some change... however I really think that the failing multiple class checking is a bug?

@simeydotme simeydotme reopened this Apr 18, 2016
@simeydotme simeydotme closed this Apr 18, 2016
@shshaw
Copy link
Collaborator

shshaw commented Apr 18, 2016

It was my own implementation of hasClass, likely adapted from my old halfDollar library, and you are correct, it does not check for multiple classes. toggleClass, addClass and removeClass do all handle multiple classes, so I can see where that may be an issue.

jQuery doesn't document having the ability to check for multiple in their hasClass and I've never used hasClass to check for multiple classes. What is the expected behavior? Is it 'has all' ( x AND y AND z ) or 'has any' ( x OR y OR z )? Looking at your code, it looks like it's "has all", I just want to be clear.

@simeydotme
Copy link
Collaborator Author

Thanks for getting back, @shshaw 😀
Yeh, that's right; jQuery doesn't support this so it'd be nice to have another 1up there... if we look at it pragmatically;

  • .addClass("big bold red") adds all classes
  • .removeClass("big bold red") removes all classes
  • .toggleClass("big bold red") toggles all classes

so it seems to make sense that:

  • .hasClass("big bold red") checks all classes

imagine something like:

var classes = "big bold red";
var $btn = $("button");

$btn.toggleClass( classes , $btn.hasClass( classes ) );

having it return true for just one of the classes would be strange.

@shshaw
Copy link
Collaborator

shshaw commented Apr 18, 2016

I wonder if it might be best to have these as separate methods. hasAnyClass('x y z'), or hasAllClass('x y z')?

@simeydotme
Copy link
Collaborator Author

well that's a logical step, but then it seems like we're just shortening the user's work for:

( $.hasClass("x") || $.hasClass("y") || $.hasClass("z") )

or

( $.hasClass("a") && $.hasClass("b") && $.hasClass("c") )

So i'd say that's not work we need to do.
Perhaps it's a design question and ken should weigh in... my thoughts are just consistency across similar interfaces with slight efficiency gain when possible 😁

@shshaw
Copy link
Collaborator

shshaw commented Apr 18, 2016

It's definitely a shortcut, but so are all of these methods 😄
We'd need to consider the amount of weight added for the methods vs the benefits. If these checks are a common need, then implementing them is worthwhile.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants