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

add toggleClass parameter for setClassToggle #421

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kasparsj
Copy link
Contributor

@kasparsj kasparsj commented Oct 8, 2015

when set to true will use toggleClass, rather than addClass/removeClass

@janpaepke
Copy link
Owner

Hi @kasparsj !

Why should this be an option? In theory both behaviours should be the same, no?
If jquery toggle logic is better maybe remove addclass/removeclass alltogether?

@kasparsj
Copy link
Contributor Author

Hi @kasparsj !

Why should this be an option? In theory both behaviours should be the same, no?
If jquery toggle logic is better maybe remove addclass/removeclass alltogether?

It's a long time since... but I think, I found it more useful to use toggleClass, maybe there was no other way to detect whether the class is on or off.
I think these are different behaviours actually, and cannot do what the other can...

@janpaepke
Copy link
Owner

I think these are different behaviours actually, and cannot do what the other can...

Could you elaborate? How exactly are the behaviours different?

@kasparsj
Copy link
Contributor Author

kasparsj commented Oct 23, 2018

the first will always "addClass" on "enter", and "removeClass" on "leave".

the second can do opposite (remove on enter, add on leave), and it can also work in conjunction with another logic (if another logic is also toggling the classes on/off).

@janpaepke
Copy link
Owner

janpaepke commented Oct 23, 2018

mh you raise a very good point.

But shouldn't this be default behaviour? Unless I'm forgetting something the current behaviour would still work with your toggle-class function.
So replacing it would be a non-breaking change, right?
What do you think?

@kasparsj
Copy link
Contributor Author

Yes, I think it should be the default behaviour, but maybe having the current behaviour can also be handy in certain cases.

@janpaepke
Copy link
Owner

but that's kind of my point. I get that using "real" toggle adds functionality, as you could invert the logic.
But I don't think entirely replacing the current function would remove anything, as it would still work the same.
I'm trying to think of a scenario where you don't want toggle and actually want to use add and remove class for whatever reason. I can't think of any. Can you?

@kasparsj
Copy link
Contributor Author

No, I can't think of any good example right now...

@janpaepke
Copy link
Owner

I think we should change the behaviour alltogether then.
I'll put it on the list for the next release.
This will also mean some documentation adjustments so won't be asking this of you. :)
I'll leave this open until I get around to attend to it.

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

Successfully merging this pull request may close these issues.

2 participants