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

Pass array of classes to `.addClass()`, `removeClass()`, and `toggleClass()` #3532

Closed
t-kelly opened this issue Feb 6, 2017 · 14 comments
Closed

Comments

@t-kelly
Copy link

@t-kelly t-kelly commented Feb 6, 2017

Description

It would be great if these examples:

var class1 = 'class-one';
var class2 = 'class-two';

$('.element').addClass(class1 + ' ' + class2);
$('.element').addClass(class1).addClass(class2);
$('.element').addClass([class1, class2].join(' '));

could instead be written like:

var class1 = 'class-one';
var class2 = 'class-two';

$('.element').addClass([class1, class2]);
@timmywil
Copy link
Member

@timmywil timmywil commented Feb 6, 2017

For the most part, we're not looking to add signatures or new features to jQuery, but I don't hate this. It has parity with libraries like React and Angular.

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Feb 6, 2017

Note that this can already be done by $('.element').addClass([class1, class2].join(" ")); which isn't that burdensome. It will take more bytes than that to do this inside jQuery.

@mgol
Copy link
Member

@mgol mgol commented Feb 6, 2017

@dmethvin

Note that this can already be done by $('.element').addClass([class1, class2].join(" ")); which isn't that burdensome. It will take more bytes than that to do this inside jQuery.

It's true but it's also uglier. :)

I like this proposal because:

  1. We don't currently accept arrays as any parameter to class methods so there should be no confusion. Because of that, it shouldn't also add that much code to jQuery.
  2. Accepting classes will align us more with the classList standard and aligning with standards is A Good Thing.
  3. It will allow to pass unprocessed class arrays to jQuery from other libraries/frameworks that use the array syntax.
  4. (This one is a bit of a stretch, but...) If we ever drop IE and start using classList instead of classNames everywhere, this would be a more natural signature and the space-separated values would be the ones that requires more pre-processing.
@timmywil timmywil removed the Needs review label Feb 6, 2017
@timmywil timmywil self-assigned this Feb 6, 2017
@timmywil timmywil added the Attributes label Feb 13, 2017
@timmywil timmywil modified the milestones: 4.0.0, 3.2.0 Feb 13, 2017
@timmywil timmywil modified the milestones: 3.2.0, 3.3.0 Feb 27, 2017
@niltoncsr
Copy link
Contributor

@niltoncsr niltoncsr commented Mar 30, 2017

I'd like to contribute and get this one! 😃
What do you think, @timmywil?

@RUPOJS
Copy link

@RUPOJS RUPOJS commented May 12, 2017

Hi @timmywil, is this already assigned to someone ? I want to take this up.

@MateusVeloso
Copy link

@MateusVeloso MateusVeloso commented May 15, 2017

Hi @timmywil, I want to take this up.

@timmywil timmywil added the Blocker label Jun 19, 2017
@jbedard
Copy link
Contributor

@jbedard jbedard commented Jun 19, 2017

Should this add support for arrays (addClass(["a", "b"])) or multiple arguments (addClass("a", "b")), or both? The multiple arguments one is more like the classList methods so that would be my vote.

@t-kelly
Copy link
Author

@t-kelly t-kelly commented Jun 19, 2017

Multiple arguments might conflict with .toggleClass()'s second boolean argument 🤔

@jbedard
Copy link
Contributor

@jbedard jbedard commented Jun 19, 2017

Seems like classList.toggle doesn't support multiple classes, only add/remove support it. I still vote to do the same as classList and don't support it with toggleClass...

@mgol
Copy link
Member

@mgol mgol commented Jun 19, 2017

@jbedard We've discussed it already (you were present :)), accepting positional parameters would require way more code and would align worse with the standard because of funkiness like .addClass('a b c', 'd'); the discussion can be seen at https://gitter.im/jquery/meeting/archives/2017/02/06.

@jbedard
Copy link
Contributor

@jbedard jbedard commented Jun 19, 2017

That was a long time ago! That info should be put in this ticket for those wanting to pick it up...

@faisaliyk
Copy link

@faisaliyk faisaliyk commented Jul 16, 2017

If no one has worked on it, can I take a stab at it?

@timmywil
Copy link
Member

@timmywil timmywil commented Jul 17, 2017

Anyone who would like to submit a PR for this, please feel free. Our contributing guidelines should answer any questions you might have about contributing to jQuery. Specifically, I suggest having a look at the Commits and PR guidelines and JS style guide.

@faisaliyk
Copy link

@faisaliyk faisaliyk commented Jul 17, 2017

Working on it.

timmywil added a commit to timmywil/jquery that referenced this issue Jan 2, 2018
+30 bytes instead of +182

Thanks to @faisaliyk for the first pass on this feature.

Fixes jquerygh-3532
Close jquerygh-3727
@timmywil timmywil closed this in 80f57f8 Jan 8, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 7, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

9 participants
You can’t perform that action at this time.