Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

fix(jqLite): properly toggle multiple classes #6448

Closed
wants to merge 1 commit into from
Closed

fix(jqLite): properly toggle multiple classes #6448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 8 additions & 3 deletions src/jqLite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,10 +840,15 @@ forEach({
removeClass: jqLiteRemoveClass,

toggleClass: function(element, selector, condition) {
if (isUndefined(condition)) {
condition = !jqLiteHasClass(element, selector);
if (selector) {
forEach(selector.split(' '), function(className){
var classCondition = condition;
if (isUndefined(classCondition)) {
classCondition = !jqLiteHasClass(element, className);
}
(classCondition ? jqLiteAddClass : jqLiteRemoveClass)(element, className);
});
}
(condition ? jqLiteAddClass : jqLiteRemoveClass)(element, selector);
},

parent: function(element) {
Expand Down
49 changes: 49 additions & 0 deletions test/jqLiteSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -666,6 +666,55 @@ describe('jqLite', function() {
expect(jqLite(b).hasClass('abc')).toEqual(false);

});

it('should allow toggling multiple classes without a condition', function () {
var selector = jqLite([a, b]);
expect(selector.toggleClass('abc cde')).toEqual(selector);
Copy link
Contributor

Choose a reason for hiding this comment

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

always use toBe instead of toEqual unless you mean it.

expect(jqLite(a).hasClass('abc')).toEqual(true);
expect(jqLite(a).hasClass('cde')).toEqual(true);
expect(jqLite(b).hasClass('abc')).toEqual(true);
expect(jqLite(b).hasClass('cde')).toEqual(true);

expect(selector.toggleClass('abc cde')).toEqual(selector);
expect(jqLite(a).hasClass('abc')).toEqual(false);
expect(jqLite(a).hasClass('cde')).toEqual(false);
expect(jqLite(b).hasClass('abc')).toEqual(false);
expect(jqLite(b).hasClass('cde')).toEqual(false);

expect(selector.toggleClass('abc')).toEqual(selector);
expect(selector.toggleClass('abc cde')).toEqual(selector);
expect(jqLite(a).hasClass('abc')).toEqual(false);
expect(jqLite(a).hasClass('cde')).toEqual(true);
expect(jqLite(b).hasClass('abc')).toEqual(false);
expect(jqLite(b).hasClass('cde')).toEqual(true);

expect(selector.toggleClass('abc cde')).toEqual(selector);
expect(jqLite(a).hasClass('abc')).toEqual(true);
expect(jqLite(a).hasClass('cde')).toEqual(false);
expect(jqLite(b).hasClass('abc')).toEqual(true);
expect(jqLite(b).hasClass('cde')).toEqual(false);
});

it('should allow toggling multiple classes with a condition', function () {
var selector = jqLite([a, b]);
expect(selector.toggleClass('abc cde', true)).toEqual(selector);
expect(jqLite(a).hasClass('abc')).toEqual(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

these tests don't prove that the condition value overrides the presents or absence of the classes on these elements. I suggest that I toggle one of the classes on a before calling the multi-toggle.

expect(jqLite(a).hasClass('cde')).toEqual(true);
expect(jqLite(b).hasClass('abc')).toEqual(true);
expect(jqLite(b).hasClass('cde')).toEqual(true);

expect(selector.toggleClass('abc cde', false)).toEqual(selector);
expect(jqLite(a).hasClass('abc')).toEqual(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

the same as above

expect(jqLite(a).hasClass('cde')).toEqual(false);
expect(jqLite(b).hasClass('abc')).toEqual(false);
expect(jqLite(b).hasClass('cde')).toEqual(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

and of course use toBe everywhere :)

});

it('should not break for null / undefined selectors', function () {
var selector = jqLite([a, b]);
expect(selector.toggleClass(null)).toEqual(selector);
expect(selector.toggleClass(undefined)).toEqual(selector);
});
});


Expand Down