Skip to content

Attributes: .removeClass() -> .attr( "class", "" ) #2465

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.

Sign up for GitHub

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

Jump to bottom
Closed
mr21 wants to merge 1 commit into from
Closed

Attributes: .removeClass() -> .attr( "class", "" ) #2465

mr21 wants to merge 1 commit into from

Conversation

@mr21
Copy link
Contributor

@mr21 mr21 commented Jul 11, 2015

Hi :)

After asking a question about a specific part of .removeClass on IRC and after @Krinkle told me that the call of .removeClass() without any arguments cleaning all the "class" attribute. I watch the code and I've found that this particularity made the code different compared to .addClass.
And if you call this.attr( "class", "" ) directly in a if we can save 17 bytes after gzipped.

I add another change in another commit to save other 2 bytes on .hasClass.

@timmywil
Copy link
Member

timmywil commented Jul 11, 2015

I like it.

@mr21
Copy link
Contributor Author

mr21 commented Jul 12, 2015

:)

After reviewing the code, I commit again to save 7 bytes on .addClass / .removeClass by removing the len variables.

markelog
markelog reviewed Jul 13, 2015
View reviewed changes
src/attributes/classes.js
Copy link
Member

@markelog markelog Jul 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder why it was done in the first place

Copy link
Contributor Author

@mr21 mr21 Jul 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, it was for removing all the classes when we call .removeClass() without any argument, but I write a special if (with the .attr) only for this case.

Copy link
Member

@dmethvin dmethvin Jul 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably to save the function call? Probably not worth the extra code. The trimming was added in e76ba32 to avoid leading/trailing space.

Copy link
Contributor Author

@mr21 mr21 Jul 13, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's not for saving a function call, it was a specific behaviour, if the value of the user is undefined we set an empty class attribute. It is really more simple to read the specific case of .removeClass() now.

@mgol
Copy link
Member

mgol commented Sep 8, 2015

@mr21 Could you rebase it over latest master (we have a new JSCS in there and jsdom shouldn't cause problems anymore), invoke npm install (the grunt should work without problems you had afterwards) and then try what @markelog suggested in his comments?

@mgol mgol added the Needs info label Sep 8, 2015
@markelog
Copy link
Member

markelog commented Oct 8, 2015

@mr21 friendly ping

@mr21
Copy link
Contributor Author

mr21 commented Oct 10, 2015

Oh sorry, I was extremely focused on something else this time.
So, no more conflict, and we are at -23.

I've tested the @markelog's version but it's not saving anything, probably because the elem variable has to be the second declaration for compression.
I've tested also the nice @Krinkle's version, but (now) it's adding +7 bytes :/

Anyway, the remove of toggleClass( bool ) will save a lot of bytes I think.

@mr21 mr21 force-pushed the classes-better-compressibility branch from 5b6ab7f to 4aa5911 Compare October 11, 2015 08:22
@timmywil timmywil closed this in 5db1e05 Oct 18, 2015
gibson042 pushed a commit that referenced this pull request Oct 25, 2015
@mr21 @gibson042
Attributes: removeClass() -> attr("class", "")
f5328b6 
- Classes simpliciation

Close gh-2465

(cherry picked from commit 5db1e05)

Conflicts:
	src/attributes/classes.js
@lock lock bot locked as resolved and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@mr21 @timmywil @mgol @markelog @Krinkle @dmethvin @gibson042 @jquerybot