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

added array feature to addClass, removeClass and toggleClass #3727

Closed
wants to merge 9 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@faisaliyk

faisaliyk commented Jul 18, 2017

Summary

#3532
#addClass, removeClass & toggleClass now accepts an array of class names as well as being backward compatible and accepting space separated class names in a single string argument.

Checklist

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Jul 18, 2017

@faisaliyk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @timmywil, @markelog and @dmethvin to be potential reviewers.

mention-bot commented Jul 18, 2017

@faisaliyk, thanks for your PR! By analyzing the history of the files in this pull request, we identified @timmywil, @markelog and @dmethvin to be potential reviewers.

@jsf-clabot

This comment has been minimized.

Show comment
Hide comment
@jsf-clabot

jsf-clabot Jul 18, 2017

CLA assistant check
All committers have signed the CLA.

jsf-clabot commented Jul 18, 2017

CLA assistant check
All committers have signed the CLA.

@mgol

Thanks for the PR!

In it's current version it's way too big, though; it adds 182 bytes to the minified gzipped build (you'll see that if you run grunt). The diff should be very small, e.g. in addClass there's just one place where we create an array by splitting a space-separated class list (I added a comment there); that assignment can be skipped in the case where we already get an array but the rest of the code should be fine; no need to repeat it.

Also, make sure to run grunt locally and you'll see your changes don't follow the jQuery style guide, there are a lot of issues to fix.

You'll also need to add tests for the feature.

@@ -23,9 +39,7 @@ jQuery.fn.extend( {
} );
}
if ( typeof value === "string" && value ) {
classes = value.match( rnothtmlwhite ) || [];

This comment has been minimized.

@mgol

mgol Jul 18, 2017

Member

Arrays should just have this assignment skipped in favor of using the first parameter as classes and the rest of the code can then stay the same...

@mgol

mgol Jul 18, 2017

Member

Arrays should just have this assignment skipped in favor of using the first parameter as classes and the rest of the code can then stay the same...

This comment has been minimized.

@faisaliyk

faisaliyk Jul 19, 2017

I used this code to make it backward compatible. So if instead of an array a string argument is passed to the method, value.match( rnothtmlwhite ) || []; will assing the string parameters as an array to classes which the proceeding code can use it to add or remove classes

@faisaliyk

faisaliyk Jul 19, 2017

I used this code to make it backward compatible. So if instead of an array a string argument is passed to the method, value.match( rnothtmlwhite ) || []; will assing the string parameters as an array to classes which the proceeding code can use it to add or remove classes

This comment has been minimized.

@mgol

mgol Jul 19, 2017

Member

It has to be backwards compatible but that is still possible without duplicating the code.

@mgol

mgol Jul 19, 2017

Member

It has to be backwards compatible but that is still possible without duplicating the code.

This comment has been minimized.

@mgol

mgol Jul 21, 2017

Member

I've looked at the latest version. How many bytes does it add compared to the current master? You'll see that when you run grunt, the minified gzipped.

@mgol

mgol Jul 21, 2017

Member

I've looked at the latest version. How many bytes does it add compared to the current master? You'll see that when you run grunt, the minified gzipped.

This comment has been minimized.

@faisaliyk

faisaliyk Jul 22, 2017

the current jquery.min.js is 85.1 KB

@faisaliyk

faisaliyk Jul 22, 2017

the current jquery.min.js is 85.1 KB

This comment has been minimized.

@mgol

mgol Jul 23, 2017

Member

What's important is the difference (in bytes) to what's currently in master. To check that you need to fire grunt on master and then on your PR.

I've tried to do that but your PR currently fails on grunt because of syntax errors. Please make sure to always run grunt before pushing changes and running the tests in at least one browser to make sure they still pass.

@mgol

mgol Jul 23, 2017

Member

What's important is the difference (in bytes) to what's currently in master. To check that you need to fire grunt on master and then on your PR.

I've tried to do that but your PR currently fails on grunt because of syntax errors. Please make sure to always run grunt before pushing changes and running the tests in at least one browser to make sure they still pass.

This comment has been minimized.

@faisaliyk

faisaliyk Jul 24, 2017

87,169 bytes is the size of the master
87,258 bytes is the size of my local

I am running grunt on my local every time I hit save. The grunt checks passes. I wonder why they are failing at your end.

@faisaliyk

faisaliyk Jul 24, 2017

87,169 bytes is the size of the master
87,258 bytes is the size of my local

I am running grunt on my local every time I hit save. The grunt checks passes. I wonder why they are failing at your end.

This comment has been minimized.

@dmethvin

dmethvin Jul 25, 2017

Member

@faisaliyk The problem is that the committed files include merge conflict markers. For example see this line. Perhaps you didn't re-push to the PR branch after resolving the merge conflicts?

@dmethvin

dmethvin Jul 25, 2017

Member

@faisaliyk The problem is that the committed files include merge conflict markers. For example see this line. Perhaps you didn't re-push to the PR branch after resolving the merge conflicts?

This comment has been minimized.

@faisaliyk

faisaliyk Jul 28, 2017

Thanks, @dmethvin. The markers have been removed. I am using VS Code. and for some reason the tab size in the code seems correct and uniform throughout, JShint and grunt passes. However, after the push, the uploaded file shows the tab size to be exaggerated. Any idea how I can resolve this issue?

@faisaliyk

faisaliyk Jul 28, 2017

Thanks, @dmethvin. The markers have been removed. I am using VS Code. and for some reason the tab size in the code seems correct and uniform throughout, JShint and grunt passes. However, after the push, the uploaded file shows the tab size to be exaggerated. Any idea how I can resolve this issue?

Show outdated Hide outdated src/attributes/classes.js Outdated
Show outdated Hide outdated src/attributes/classes.js Outdated
Show outdated Hide outdated src/attributes/classes.js Outdated
Show outdated Hide outdated src/attributes/classes.js Outdated
Show outdated Hide outdated test/classArrayTest.html Outdated

timmywil added a commit to timmywil/jquery that referenced this pull request Jan 2, 2018

Attributes: allow array param in add/remove/toggleClass
+30 bytes instead of +182

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

Fixes jquerygh-3532
Close jquerygh-3727

@timmywil timmywil referenced this pull request Jan 2, 2018

Closed

Attributes: allow array param in add/remove/toggleClass #3917

4 of 4 tasks complete
@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Jan 2, 2018

Member

Thank you for this PR, but closing in favor of #3917.

Member

timmywil commented Jan 2, 2018

Thank you for this PR, but closing in favor of #3917.

@timmywil timmywil closed this Jan 2, 2018

@lock lock bot locked as resolved and limited conversation to collaborators Jul 1, 2018

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