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

$animator and ngShow: Initial state after route change #3215

Closed
olostan opened this issue Jul 12, 2013 · 20 comments
Closed

$animator and ngShow: Initial state after route change #3215

olostan opened this issue Jul 12, 2013 · 20 comments
Assignees
Milestone

Comments

@olostan
Copy link
Contributor

olostan commented Jul 12, 2013

There was in issue #2309 about animations that are run on a elements that are hidden during initial load. And it was fixed in case if page is opened straight away.

But in case if some view is loaded with ngView after route change all animations that initially shouldn't be run are run...

Here is example: http://plnkr.co/edit/lOu6Np?p=preview

May be disable animations during route change?

@blob42
Copy link

blob42 commented Aug 26, 2013

+1

@olostan
Copy link
Contributor Author

olostan commented Aug 27, 2013

Updated to 1.2 RC ( http://plnkr.co/edit/lOu6Np?p=preview ).... seems much better.

The only problem that stays is a text that should be initially hidden is shown after route change and then quickly disappear (but without animation).

So this issue could be probably closed...

@matsko
Copy link
Contributor

matsko commented Sep 5, 2013

@olostan can you please try 1.2rc2. Some flicker-related fixes were merged in.

@olostan
Copy link
Contributor Author

olostan commented Sep 10, 2013

wow.... unfortunately it becomes even worse: http://plnkr.co/edit/lOu6Np?p=preview

in rc1 text that should initially hidden just flicked, but now it is animated... so looks like hiding animation was fired to the elements that were initially hidden... :(

@dfaivre
Copy link

dfaivre commented Sep 11, 2013

Same behavior with ui-router.

@ghost ghost assigned matsko Oct 1, 2013
@matsko
Copy link
Contributor

matsko commented Oct 1, 2013

Working on a fix for pre 1.2.

@matsko
Copy link
Contributor

matsko commented Oct 1, 2013

@olostan does this fix your problem fully? http://plnkr.co/edit/nBlXzb?p=preview. This one fixes ngShow.

Avoiding ng-hide animation during page load can only work if you apply an animation to the ngView element.

@olostan
Copy link
Contributor Author

olostan commented Oct 2, 2013

@matsko I see fading-out animation after route was changed.

The issue is that if initially (in controller or ng-init) the ngShow expression is false we shouldn't apply ng-show-remove etc classes so no animation should happen at all.

I've tried some solutions but didn't found ideal one: if to turn off animation during route change, then some nice features like animated highlighting current page in breadcrumb is gone. Other solution - to track is it initial value in ngShow's $watch (check for undefined of second paramter - old value) works good but looks like not so generic (actually I did a pull request with this solution). But now, as for me, it could be quite good workaround until there is another way to figure out is it initial state of expression or not.

@kevin-smets
Copy link

👍

@matsko
Copy link
Contributor

matsko commented Oct 2, 2013

@olostan thinking about animations for the first ngShow/ngHide digest change makes me believe there is no point in animating them. Only when they change. This typically doesn't follow the function of other directives, but if you think about it then you don't see ngInclude or ngRepeat "leaving" itself when the directive boots up. I'll update the plunkr shortly.

@olostan
Copy link
Contributor Author

olostan commented Oct 2, 2013

@matsko absolutely agree - there is no point of doing any initial animation: even if initial value of ngShow expression is true and it is inside some partial view, loaded by ngView/ngInclude, appearing animation should be responsibility of ngView/ngInclude.

So that is why my idea was in https://github.com/angular/angular.js/blob/master/src/ng/directive/ngShowHide.js#L148 to do something like

    scope.$watch(attr.ngShow, function ngShowWatchAction(value, oldValue){
      if (oldValue === undefined)
         // this is probably our first digest, so we shouldn't animate it
         element[toBoolean(value) ? 'removeClass' : 'addClass']('ng-hide');
      else
         $animate[toBoolean(value) ? 'removeClass' : 'addClass'](element, 'ng-hide');
    });

But I am not sure that we can trust this test of is it first digest or not. Or it is better to:

    var watcher = scope.$watch(attr.ngShow, function ngShowWatchFirstDigestAction(value){
         // Its our first digest, so we directly add class without animation
         element[toBoolean(value) ? 'removeClass' : 'addClass']('ng-hide');
         watcher(); // unregister
         scope.$watch(attr.ngShow, function ngShowWatchAction(value){
              $animate[toBoolean(value) ? 'removeClass' : 'addClass'](element, 'ng-hide');
         }); 
    });

@matsko
Copy link
Contributor

matsko commented Oct 3, 2013

Thanks @olostan for the code. This actually effects all inner animation directives (ngRepeat/ngInclude/ngSwitch/ngIf/ngShow/ngClass) so the solution is to automatically shim the ngView animation so that no other animations are rendered per view change (even if ngView doesn't have an animation). This means that only ngView and ngInclude need to be changed and not only ngShow.

I talked with @mhevery and we're working on the fix.

@olostan
Copy link
Contributor Author

olostan commented Oct 3, 2013

But as for ngView there could be quite usefull to have initial animation. For example on one of my projects ( http://gamme.co/ I use initial animation on ngView when you just open web site.

@matsko
Copy link
Contributor

matsko commented Oct 4, 2013

ngView will have it (but only when the route first changes not on bootstrap). This basically means that any top-level structural directive will (like ngInclude/ngSwitch/ngIf/ngView and ngRepeat) will stop child the animations temporarily whenever it does its thing (anything inside of those directives will be disabled until the compilation and digest loop are done).

Here's a grand demo (no animation present on ngView):
http://plnkr.co/edit/nBlXzb?p=preview

Here's a grand demo (animation is present on ngView):
http://plnkr.co/edit/YGmknb?p=preview

@olostan
Copy link
Contributor Author

olostan commented Oct 4, 2013

@matsko Great! Waiting for commit in master branch :)

@kevin-smets
Copy link

@matsko awesome work, can't wait to pull in the changes 👍

@mstachowiak
Copy link

Agreed, great work @matsko. We've been testing out the demo code for ngAnimate in our dev environment and it's been performing as expected. No issues to report and the ngShow/ngHide problems are resolved.

@matsko
Copy link
Contributor

matsko commented Oct 5, 2013

OK great. This fixing a few more things and then I'll have it up in master.

matsko added a commit to matsko/angular.js that referenced this issue Oct 11, 2013
… even if no animation is present during compile

Closes angular#3215
@matsko matsko closed this as completed in cc58460 Oct 11, 2013
@matsko
Copy link
Contributor

matsko commented Oct 11, 2013

Landed as cc58460

@kevin-smets
Copy link

Awesome, tested and working on ipad. 👍

jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
… even if no animation is present during compile

Closes angular#3215
jamesdaily pushed a commit to jamesdaily/angular.js that referenced this issue Jan 27, 2014
… even if no animation is present during compile

Closes angular#3215
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.

6 participants