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

Update ngClass.js #7479

Closed
wants to merge 1 commit into from
Closed

Conversation

trevordowdle
Copy link

I don't know if this was intended, but I don't think the code was comparing the indexes correctly and it was causing some weird behavior when using the ng-class-even and ng-class-odd directives.

See the following plunkr for an example of the bug. http://plnkr.co/edit/VAi6DB0QdWRutNGnVdSL?p=preview

Select the Red Radio button option to see that when switching classes on rows the classes are not always switched properly.

I don't know if this was intended, but I don't think the code was comparing the indexes correctly and it was causing some weird behavior when setting classes on ng-class-even and ng-class-odd.

See the following plunkr for an example of the bug.  http://plnkr.co/edit/VAi6DB0QdWRutNGnVdSL?p=preview

Select the Red Radio button option to see that when switching classes on rows the classes are not always switched properly.
@mary-poppins
Copy link

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 (#7479)

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!

@petebacondarwin
Copy link
Contributor

@trevordowdle - thanks for this PR. Could you create a unit test that demonstrates the bug and sign the CLA please?

@petebacondarwin petebacondarwin added this to the 1.3.0 milestone May 15, 2014
@trevordowdle
Copy link
Author

I just signed the CLA.

I would be happy to create a unit test. However my wife is pregnant and in the Hospital getting ready for labor and I need get there right now. So It may be a 2-3 days but as soon as I have it, i'll get it to you.

What should I do with the unit test once created? Is there a place in the repository you store unit tests?

Thanks

@btford
Copy link
Contributor

btford commented May 15, 2014

@trevordowdle no rush. and congrats. :)

You could add a test here: https://github.com/angular/angular.js/blob/master/test/ng/directive/ngClassSpec.js

See the development docs for running the tests:
https://docs.angularjs.org/misc/contribute#running-the-unit-test-suite

@shahata
Copy link
Contributor

shahata commented May 16, 2014

This is indeed a bug, I already have a PR for this: #7262

@btford
Copy link
Contributor

btford commented May 16, 2014

I'm closing this in favor of #7262. Still, thank you @trevordowdle!

@btford btford closed this May 16, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants