feat(ngClass): support multiple classes in key

[shahata]

fixes #7135

[mary-poppins]

Thanks for the PR! Please check the items below to help us merge this faster. See the contributing docs for more information.

• Uses the issue template (#7140)

If you need to make changes to your pull request, you can update the commit with git commit --amend.
Then, update the pull request with git push -f.

Thanks again for your help!

[btford]

This is a feature, not a fix.

[btford]

I doubt we want this... @IgorMinar @caitp: thoughts?

[caitp]

I dunno, it's not a whole lot of code, and it might make things behave more in a way people expect, but yeah I am not sure.

[shahata]

@btford I regard this as a fix since the test would have passed if we ran it before your commit was added and as you can see in the comments, ppl used this "undocumented feature"

I am referring to commit 55fe6d6

[pkozlowski-opensource]

I guess we should merge this one as there are more people bumping into it, ex:
#7393

[caitp]

Yeah, since it's a regression, it probably should be fixed

[caitp]

This generally LGTM.

[caitp]

should prefer toBe(true/false) over toBeTruthy/Falsy(), though

[shahata]

I just went with the flow. toBeTruthy/Falsy() is used all over ngClassSpec. I can fix them all if you like, np...

[shahata]

There's a custom matcher which is already partially used: toHaveClass. We could change all tests to use it... WDYT?

[caitp]

I don't think it's worth changing the tests to do that, but you could certainly use that here, it would be better semantically.

It has an issue with testing for multiple classes though --- if they're in the wrong order, it gets them wrong, and there are other issues with it. But it reads nicely

[shahata]

ok, fixed only for this single test

[btford]

It has an issue with testing for multiple classes though

The custom matcher does? It looks like all it does is call hasClass internally.

[caitp]

I mean specifically if you are testing multiple classes in the same call, like so:

expect(element).toHaveClass(‘foo bar baz’);

The reason is that what it’s essentially doing is checking className.indexOf(expectedClass), so if they’re in the wrong order, it breaks.

It doesn’t hurt these tests, though.

Caitlin Potter
caitpotter88@gmail.com

On May 30, 2014, at 2:59 PM, Brian Ford notifications@github.com wrote:

In test/ng/directive/ngClassSpec.js:

@@ -205,6 +205,23 @@ describe('ngClass', function() {
expect(e2.hasClass('odd')).toBeTruthy();
}));

• it('should allow ngClass with overlapping classes', inject(function($rootScope, $compile, $animate) {

•  element = $compile('<div ng-class="{\'same yes\': test, \'same no\': !test}"></div>')($rootScope);
  

•  $rootScope.$digest();
  

•  expect(element.hasClass('same')).toBeTruthy();
  

•  expect(element.hasClass('yes')).toBeFalsy();
  

•  expect(element.hasClass('no')).toBeTruthy();
  

  It has an issue with testing for multiple classes though

The custom matcher does? It looks like all it does is call hasClass internally.

—
Reply to this email directly or view it on GitHub.

[shahata]

So this is a general issue with hasClass. We could fix toHaveClass to split by space and test them individually. Doesn't seem worth it imo, probably better to have multiple assertions in the actual test.

[CesarAfonso]

Agreed. 👍
I ran into this today after migrating to 1.2.16

I use this to add dynamic behavior into html elements by choosing one set of classes. In which set, some classes may or may not be repeated.
My example : http://jsfiddle.net/greenhawk/bS4nV/

[btford]

@shahata fair enough; @caitp I'm going to make these changes to the tests and merge this.

[btford]

Landed as 7eaaca8.