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

ngClass in 1.2.0-rc1 with animations awkward #3587

Closed
OverZealous opened this issue Aug 14, 2013 · 22 comments
Closed

ngClass in 1.2.0-rc1 with animations awkward #3587

OverZealous opened this issue Aug 14, 2013 · 22 comments
Milestone

Comments

@OverZealous
Copy link

I think the animating of ngClass doesn't make sense. With 1.2.0-rc1, adding or removing a class works in a completely non-intuitive manner. Before it was easy to use class adding and removal to perform simple CSS-based animations: add a class, handle the CSS changes in the CSS. No problem.

Now, however, ngAnimate ends up waiting to add the new class until the animation has run, then applies the new class, then the actual animation happens. As an example, assume you had this:

.foo {
    color: white;
    transition: 5s ease all;
}

.foo.active {
    color: red;
}

And in your view:

<span class="foo" ng-class="{active: fooEnabled}">Hello World</span>

Previously, the color would animate immediately upon setting fooEnabled to true. Now, there's a 5 second pause, then the animation occurs.

As a workaround, I've found that you can set the class on a parent object, and only have the transition on a child, but this is really impractical.

I think there either needs to be a way to prevent ngClass from animating or disable animations on ngClass altogether. It really doesn't make sense to add animations to ngClass, though.

@ryanzec
Copy link

ryanzec commented Aug 16, 2013

I have to agree that animating ng-class is not every class in ng-class should be animated (but it seems like they are). I am having issues where there is a noticeable delay is classes being added. For instance I have a ng-show and ng-class that are both using the same variable on the same element. Even though that element has no animations or transitions applied to it through css, the class is still being animated which means there is the chance that the element will show however the class is not added because it is being animate.

I think having ng-animate was good. It acts just like ng-class expect it animates the classes. Just because something has ng-show, ng-hide, ng-if, etc... does not 100% mean that the class should be animated. In simple examples it only happens once in a while but in more complex application, it is a consistent issue.

@ioagel
Copy link

ioagel commented Aug 19, 2013

I also have problem with ngClass animating by default. I use ngClass to show/hide icon-spinner(font-awesome) during a rest call, but now with angularjs 1.2.0-rc1, there is a delay in hiding the spinner (when the results come back).

@bzamora
Copy link

bzamora commented Aug 19, 2013

Same issue here. Was using ngClass to flag a menu item as selected, now there is a delay with the selected animation and opening the menu. The effect before was the selected animation and opening of menu happened at same time, now there is a delay. Similar to this menu: http://tympanus.net/codrops/2013/07/30/google-nexus-website-menu/

@ryanzec
Copy link

ryanzec commented Aug 19, 2013

Glad I am not the only one experiencing this issue. Just a reminder that this is just not ngClass but most directives I have found that effect display (ngShow and ngHide for example). At a minimum, there should be no animated classes on elements unless they have a CSS animation/transition on them I think (personally I had no issue with the ngAnimate directive).

Would like to get a word from the Angular team to see if they think this is an issue or if this change was the desired effect (hoping it wasn't).

@matsko
Copy link
Contributor

matsko commented Aug 19, 2013

This is a known issue and a fix is underway. What's going on is that ngAnimate thinks that the existing transition on the element is the transition inside of .ng-enter, .ng-leave, .ng-move or with a class-based animation (like foo-add). Then it animates in between with the duration even though nothing has changed and then it adds the class and then the natural animation takes place.

I'm thinking that if the base class or element (in this case .foo) already has a transition setting prior to .foo-add (or any other generated class) being attached then it doesn't perform an ngAnimate animation and lets the natural CSS animation occur.

Does anyone see any conflicts with this?

@damrbaby
Copy link
Contributor

@matsko I believe the issue I reported in #3613 and the issues reported by @bzamora and @ryanzec are one and the same.

@OverZealous
Copy link
Author

@matsko That was the solution I was thinking of, too. It's a fairly clean and declarative way to choose which sort of animation to apply.

Would it make sense for this change to be applied across the board to all items requesting $animate support?

@ryanzec
Copy link

ryanzec commented Aug 19, 2013

Would it make sense for this change to be applied across the board to all items requesting 
$animate support?

I would say yes.

@damrbaby
Copy link
Contributor

This is another plunkr that was just posted in the angular google group about buggy behavior with ng-if as a result of having ngAnimate as a dependency:

Notice the flashing in chrome:
http://plnkr.co/edit/iYMm0IHZkrRamlaEs9yD?p=preview

Original conversation:
https://groups.google.com/forum/?hl=en#!topic/angular/Fy-rVdXh5WQ

@deakster
Copy link

I started porting some of my angular projects over to 1.2.0rc1, and the one consistent issue I seem to find in a huge amount of places is various manifestations of 'flickering', similar to what is show in the example by @damrbaby above.

It's always a bit different, things like text which should be centred momentarily left aligns for a split second, or boxes of a certain size momentarily collapse to be 0 width and height. Basically visual discrepancies that seem to last a single frame or so, but still being blatantly noticeable because of the frequency and regularity.

Is it safe to assume that these 'flickering' behaviours are a result if this issue?

@damrbaby
Copy link
Contributor

@deakster I opened up another issue #3613 in regards to the flickering, and I think it's yet to be confirmed if that issue and this one are one-and-the-same.

@matsko
Copy link
Contributor

matsko commented Aug 21, 2013

The flickering bug is separate, but quite related. Lets stick to the ngClass bug for right now.

The solution is to skip animations if the given element already has a transition duration applied on it when there are no generated ng-classes present on it. This allows for both the expected usage of ngClass (natural CSS animations) as well as the ngAnimate use of ngClass (CLASS-add and CLASS-add-active). Here's a demo of the usage:
https://s3.amazonaws.com/angularjs-dev/natural-css-bug/example/animate.html#

Once I can confirm with @damrbaby that the flicker bug is fixed then we're set.

matsko added a commit to matsko/angular.js that referenced this issue Aug 23, 2013
…ady has transitions/durations attached to it

Closes angular#3587
@matsko matsko closed this as completed in 7c605dd Aug 23, 2013
@matsko
Copy link
Contributor

matsko commented Aug 23, 2013

Landed as 7c605dd

@ryanzec
Copy link

ryanzec commented Aug 27, 2013

I created a custom build and for at least the 2 issues I was seeing, they are fixed with the new code. I would be interested if anyone else is still experiencing any issues after this fix as I only had a limited number of issues (my application is only a few pages right now).

@mbleigh
Copy link

mbleigh commented Oct 25, 2013

I have an example of when you would NOT want this behavior: when you have transitions and animations that are applied separately to an element. This demo exemplefies: http://jsbin.com/usaruce/236/edit

There is a transition on the element generally for color, but a separate animation that, in rc2 was applied by Angular. In rc3 this is skipped, which is not desired behavior, because the transition is already applied but as a wholly separate concern.

@deakster
Copy link

@mbleigh This has been causing a lot of issues for me too. I was able to get around some of them by wrapping the elements in another parent element, and having the ngRepeat animations on the parent and the other secondary animation on child elements, but not only did this complicate my DOM, it is not possible to re-create certain types of animations in this way.

For example, I have a layout which is basically a tile editor. I need the ngRepeat animations to show and hide the tiles, but I also had transitions to animate the width & height of the tiles, which are not triggered by ngRepeat, but determined based on other classes, for example .small, .medium, .large each has a different width and height. This is basically impossible to do now since RC3.

@matsko
Copy link
Contributor

matsko commented Oct 26, 2013

@deakster @mbleigh I've been thinking about this and I've come to a decision. The decision is to change the behaviour of ngClass animations such that the active class is added/remove at the same time as is the target class when ngClass changes. This way $animate doesn't have to do that pre-check operation to see if the base class already contains animations.

This way your CSS code can look like:

.repeater {
  transition:1s linear all;
}

.repeater.ng-enter-active {}
.repeater.ng-leave-active {}
.repeater.off, .repeater.off-add-active {}
.repeater:not(.off), .repeater.off-remove-active {}

I think this is the solution. The only issue is that it hinders the JS API (the 1.2 version) and it would mean that it too would have to be changed so that animations can be performed before and after the DOM opreation (enter, leave, move, addClass, removeClass).

Unfortunately with the release of 1.2 next week, there's no way I can get this done. But you can rest assured that I will modify the CSS/JS APIs once 1.2 is out so this can be ready for 1.3.1 (1.3 should be the next unstable branch).

@deakster
Copy link

@matsko That would be great. I would be happy to switch to that code even before it makes it into an official release, please let us know once you have that in a commit or custom build.

@matsko
Copy link
Contributor

matsko commented Nov 6, 2013

@deakster this is in master now :)

@deakster
Copy link

deakster commented Nov 6, 2013

@matsko Excellent, I will give it a shot shortly, thanks.

@ryanzec
Copy link

ryanzec commented Nov 6, 2013

Is this going to be in 1.2.0 or still 1.3.1? I only ask as 1.2.0 has not yet been released and was wondering if there was going to be another RC release that this might be able to get into?

@matsko
Copy link
Contributor

matsko commented Nov 6, 2013

1.2.0. Use http://code.angularjs.org/snapshot/ until 1.2.0 is released within the next day or so.

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants