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

[1.2.3] bug in ng-class-odd with multiple words in class name #5271

Closed
elgs opened this issue Dec 4, 2013 · 13 comments
Closed

[1.2.3] bug in ng-class-odd with multiple words in class name #5271

elgs opened this issue Dec 4, 2013 · 13 comments

Comments

@elgs
Copy link

elgs commented Dec 4, 2013

I believe this is a new bug introduced in Angularjs 1.2.3, please see the following JSFiddle.
http://jsfiddle.net/QSr2F/

When there are multiple words in the ng-class-odd/ng-class-even, the first and second rows are ok, starting from the third row, the class for the odd rows is wrong, while the class for the even rows is right.

There is no such problem when the class names are single word (with no spaces in between).

@caitp
Copy link
Contributor

caitp commented Dec 4, 2013

http://jsfiddle.net/QSr2F/1/ --- Works just fine

@elgs
Copy link
Author

elgs commented Dec 4, 2013

@caitp Thanks for reply. It seems the behavior changed since 1.2.2.
Here is your version of the code:

<span class="ui message" ng-class-odd="'odd'" ng-class-even="'even'">

And here is mine:

<span ng-class-odd="'ui odd message'" ng-class-even="'ui even message'">

Does it mean I have to move the common parts in the class to a separate class. It's generally not a problem if the class names are static. However, if they are dynamic, it's going to be tricky to figure out the 'common parts'.

Don't know if this is intended as a new feature, or it's just a bug?

@ghost ghost assigned tbosch Dec 4, 2013
@caitp
Copy link
Contributor

caitp commented Dec 4, 2013

I don't believe this is a bug, I'm fairly sure this is part of a change to help ngAnimate + ngClass not be broken. Matias would know better, but I expect that's what you'd find if you used git bisect to see where the change happened.

@tbosch
Copy link
Contributor

tbosch commented Dec 4, 2013

According to the docs this should work like @elgs used it: http://docs.angularjs.org/api/ng.directive:ngClassEven says "The result of the evaluation can be a string representing space delimited class names or an array." Same for ngClassOdd.

@elgs
Copy link
Author

elgs commented Dec 4, 2013

Thanks @caitp, at least your code is a work around for me. OK, if this is not a bug, at least when the code is written like mine, the ng-class-odd behaves semantically inconsistently. Is my statement fair?

@caitp
Copy link
Contributor

caitp commented Dec 4, 2013

I am not contesting that @tbosch, and I think that it is fair to expect this behaviour @elgs.

However, if this is related to the ngAnimate + ngClass brokenness from a while ago, I'm not sure we can just revert the change, that would be bad :( So a fix would be a bit more involved.

I don't want to ping le animation expert since he seems to be busy with other things at the moment, but he might have an idea of how to solve it quickly

@elgs
Copy link
Author

elgs commented Dec 4, 2013

+1 to @tbosch.

It's ok for me to use @caitp 's code as a workaround now, and I don't expect you to revert the change just in order to fix this problem. I just hope the devs are aware of this issue, and some day it can be fixed.

I think this is going to be a problem when the class names are dynamically generated.

@caitp
Copy link
Contributor

caitp commented Dec 4, 2013

elgs: you should be able to just use the regular ngClass for items which don't care about $index, it should still play along with ngClassEven and ngClassOdd --- or even ng-attr-class

@elgs
Copy link
Author

elgs commented Dec 4, 2013

@caitp yes, that's right.

However, what if when the odd and even classes are dynamically generated at run time, and they happen to share some common words?

@caitp
Copy link
Contributor

caitp commented Dec 4, 2013

Your goal for the time being should be to keep the directives from fighting with each other, I think. Try not to duplicate class names across them

@ghost ghost assigned matsko Dec 9, 2013
@matsko
Copy link
Contributor

matsko commented Dec 10, 2013

@elgs you'll run into problems if your code is removing and adding the same CSS class during two animations. The ideal code for is just as provided above:

<span class="ui message" ng-class-odd="'odd'" ng-class-even="'even'">

@tbosch tbosch modified the milestones: 1.2.12, 1.2.11, 1.2.13 Feb 3, 2014
@btford btford modified the milestones: 1.2.14, 1.2.13 Feb 15, 2014
@IgorMinar IgorMinar modified the milestones: 1.3.0-beta.1, 1.2.14 Mar 1, 2014
@btford btford modified the milestones: 1.3.0-beta.2, 1.3.0-beta.1 Mar 10, 2014
@tbosch tbosch modified the milestones: 1.3.0-beta.3, 1.3.0-beta.2 Mar 14, 2014
@btford btford modified the milestones: 1.3.0-beta.4, 1.3.0-beta.3 Mar 24, 2014
@btford
Copy link
Contributor

btford commented Apr 2, 2014

I'm so on this.

@matsko
Copy link
Contributor

matsko commented Apr 3, 2014

Fixed with @btford's amazing patch: http://jsfiddle.net/QSr2F/7/

btford added a commit to btford/angular.js that referenced this issue Apr 3, 2014
The basic approach is to introduce a new elt.data() called $classCounts that keeps
track of how many times ngClass, ngClassEven, or ngClassOdd tries to add a given class.
The class is added only when the count goes from 0 to 1, and removed only when the
count hits 0.

To avoid duplicating work, some of the logic for checking which classes
to add/remove move into this directive and the directive calls $animate.

Closes angular#5271
@btford btford closed this as completed in c967792 Apr 3, 2014
btford added a commit that referenced this issue Apr 3, 2014
The basic approach is to introduce a new elt.data() called $classCounts that keeps
track of how many times ngClass, ngClassEven, or ngClassOdd tries to add a given class.
The class is added only when the count goes from 0 to 1, and removed only when the
count hits 0.

To avoid duplicating work, some of the logic for checking which classes
to add/remove move into this directive and the directive calls $animate.

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