Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Animation is now evaluated dynamically #96

Closed
wants to merge 1 commit into from

Conversation

pennersr
Copy link

I was trying to modify the view animation dynamically, e.g. by using one of:

ng-animate="scopeVariable"
ng-animate="scopeMethod()"

instead of the fixed:

ng-animate="'fade'"

While this works, the scope references are only evaluated once at link time, hence, changing those dynamically did not work.

@ksperling
Copy link
Contributor

Hmm... core ng-view only creates the animator once on link, and this is also what the ng-animate guide says to do (http://www.yearofmoo.com/2013/04/animation-in-angularjs.html#using-animatings-in-your-own-directives), so at this point I'm not really convinced that this change is the right thing to do.

@jeme
Copy link
Contributor

jeme commented Apr 22, 2013

@pennersr what is the use case, and what isn't happening the way you expected it to happen?...

Considering that we follow the core teams recomendations, it would also be really interesting this is the same behavior in core, and if it should be something that should be posted to them instead.

E.g. if your scenario is meant to work according to the core team, but actually doesn't then it could be a bug in either their directives as well or the animator implementation, and if it's the last being the case, that is why we really don't wan't to change things, as it would be a workaround to a bug, and then I would rather see the bug being fixed.

@pennersr
Copy link
Author

My use case is as follows: I would like to use ui-router for the navigation of a mobile app, where the animation is dependent on the state the user is navigating to. For example, a dialog-like view needs to slide up, whereas normal views slide in from right to left. Furthermore, if my states are ordered I can easily detect whether or the user is moving forward (right-to-left animation), or going back (left-to-right animation).

I can easily accomplish this by using a dynamic lookup for the ng-animate. For example, one could inspect the from/to state on $stateChangeStart and alter the animation accordingly. This works as long as the ng-animate state is dynamically evaluated.

Is there any other way to accomplish this?

@ksperling
Copy link
Contributor

Sounds like a valid use case, but I think we need a more obvious way of doing this.

I think $animator needs the ability to pass a user-defined 'context' object to the (JS) animation, for which we could pass the transition object with properties like 'from', 'to' etc identifying the states involved. This meta-animation could then pick the actual animation to perform -- assuming $animator was to provider some simple way of doing that.

@pennersr
Copy link
Author

Is there somewhere where we can validate whether or not my patch is really off-limits? I've read http://www.yearofmoo.com/2013/04/animation-in-angularjs.html#using-animatings-in-your-own-directives, and while it lists some sample code I have difficulties interpreting that as the only, one true way of doing things. IMHO, not picking up the animation dynamically feels rather un-angularly.

@jeme
Copy link
Contributor

jeme commented Apr 23, 2013

@pennersr if you watch the video with Msiko, I do indeed think your meant to be able to pick up these animations dynamically, that is why I am questioning if this is really an issue in the core instead. (If I remember correctly at least)

If we fix it like in your pull request, it would only be fixed for ui-view, but still be broken for ng-view, ng-include and all the others, that is why it is very interesting to me to figure out weather the issue also exists in those or not.

And if it is in reality a bug in the $animator or just in their directives as well, if their directives are the ones they wan't to fix, then we will to, but if they wan't to fix the animator for this, then doing this fix here, might even cause trouble.

@pennersr
Copy link
Author

Indeed, which is why I submitted angular/angular.js#2480
Though I am not sure whether or not submitting a ticket is the best way forward to get the issue discussed -- perhaps you guys have a more direct connection to the Angular core team?

@ajoslin
Copy link
Contributor

ajoslin commented Apr 29, 2013

Any progress on resolving this? Wouldn't it be good to patch this in for now, and then move forward with the angularjs ticket? It's not like this change requires a huge code fork; it's a small two line change. 🌴

I'd say it's good for ui-router to make opinionated decisions that contradict what angularjs core is doing wrong. It will encourage them to fix it.

@ksperling
Copy link
Contributor

@ajoslin I agree that the dynamic evaluation behaviour is desireable, but from an API point of view having to recreate the animator object before each use is just a work-around: The two-step approach of getting an animator instance from $animate and then using it is clearly designed such that the animator does not need to do all the setup work all over again for each animation. So a proper fix for this issue can really only happen in $animator.

If this issue is considered important enough we could include the workaround, and then make sure we go back to the recommended way of doing it once it's fixed in core.

@rynz
Copy link
Contributor

rynz commented May 6, 2013

Does anyone have any sample code to get around this issue? Even if it's specific to ui-router only. I too need to animate right/left, left/right depending on what was clicked/touched.

I would have thought you pass someMethod() to the ng-animate directive which returns an object of {enter: 'enter-css-class', leave: 'leave-css-class'} Is this not the case?

@pennersr
Copy link
Author

Starting since Angular 1.1.5, ng-animate is now evaluated dynamically. Example: http://jsfiddle.net/fEbgM/1/

@nateabele
Copy link
Contributor

Awesome. Do you happen to know the commit that introduced it? We can use that as the reference implementation. If you don't know off-hand, no worries, I'll look it up when I get a chance.

@pennersr
Copy link
Author

Looks like this is the commit that introduced it: angular/angular.js@fd21c75

@damrbaby
Copy link

Tested with Angular 1.1.5 on a ui-view ng-animate is now evaluated dynamically, so I think this can be closed, as there does not need to be any further implementation.

@nateabele
Copy link
Contributor

Awesome. @pennersr Can you verify on your end?

@pennersr
Copy link
Author

Confirmative, with 1.1.5 this works fine with the current ui-router. Closing...

@pennersr pennersr closed this May 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants