feat(animation): Animation library and demo application for Angular #551
Conversation
| * Add the [cssClass] to the classes on [element] after running any defined | ||
| * animations. Any existing animation running on [element] will be canceled. | ||
| */ | ||
| AnimationHandle addClass(dom.Element element, String cssClass); |
There was a problem hiding this comment.
I think this should take a List and we should remove all of the __ToAll methods
There was a problem hiding this comment.
Lets talk about this. The way the animation system is currently designed, an instance of an Animation represents the state of an animation for an individual element, so for reasoning purposes its easier to check for an individual element if an animation exists.
If a developer uses the animate service (eg, to build a popup component), most of the time they will probably want to only use the simple case and animate a single element since in most cases that's all they will actually have access too. If we remove the ___ToAll methods, an animate implementation has to manage the list of future merging individually, and anyone using the animate service will have to wrap their call in a list, which will result an extra handle object that does a Future.wait on a list of a single Future.
It might be possible to avoid the extra future, and possibly make the 'element' parameter dynamic (accept both a list and a single element), but it seemed more explicit to have two explicitly typed methods for an api a developer would use.
There was a problem hiding this comment.
After we talked about how Blocks should call animations, I am even more convinced that we should stick to List methods only. What do you think?
There was a problem hiding this comment.
I've switched over to list only based implementations. It makes the code slightly more complex, but it now passes all tests, and I've added specific tests for the list based implementations.
| * | ||
| * Note: The goal is that this will eventually be integrated with AngularDart. | ||
| */ | ||
| abstract class Animate { |
There was a problem hiding this comment.
what about a toggle<...> variant for each add/remove methods which would take an extra bool add = true arg ? This would allow simplify the calling code.
There was a problem hiding this comment.
Possibly, and it might be a nice addition, but you run the risk that your model and your dom could get out of sync if you rely on querying the dom to see if a state exists and then toggling it.
Do you have an example before/after example of how you would expect the usage to change?
There was a problem hiding this comment.
I sent a PR to your repo actually. The idea is much more simple:
toggle(..., true) = add(...)toggle(..., false) = remove(...)
There was a problem hiding this comment.
You can see the details in the codelogic#1
|
Hi everyone, there have been a significant number of updates to this pull request. The animate interface has been completely redone and animation is integrated into the blocks themselves. Please take a look and add comments, I'm sure there's a couple of things I'm missing. |
|
@codelogic I think it would be easier to review if your PR is against 'misko/change-detector'. The PR would be on an other repo but you can just link it from here. What do you think ? |
|
Before the PR target changes, using this could help with the review: |
|
Pull request has been rebased on mhevery/angular.dart/change-detector here: mhevery#9 so its easier to review. |
|
@codelogic GH allows you to set the target branch for the PR. Here you could have picked 'mhevery/change-detector' as the targret branch to see the commits related to this PR only. The negative point is that the PR would be opened on the fork of Misko. So I think this is good for now. One more thing you can not (unless it has changed recently) changed the target branch once the PR is opened, you have to send a new PR to change the target branch. |
Excludes animations called directly with 'play'.
…passed by default.
…talled module. To enable animation, you now .
|
PR has been rebased (yet again) on angular/master now that most of the change detection work by misko is now in master. |
…longest duration.
Move NgAnimate and NoAnimate to core_dom Add hooks for ng-class, ng-cloak, ng-form, ng-control, ng-model Split animation loop and animation optimizer. Combine AnimationHandle and Animation Split out AnimationOptimizer Allow multiple animations to run on the same Element Renaming of multiple animation classes. Additional specs for more pieces of code.
…prevent child animations from running.
|
Closed in favor of a much cleaner pull request: #635 |
Rough cut of animate service implementation, demo application under demo/animate_demo/index.html
Demo currently includes:
ng-ifexample that animates css opacity and height animation.animate.addCss()andanimate.removeCss()Animateservice withanimate.addCss()andanimate.removeCss().Animation library currently includes:
window.requestAnimationFramebasedAnimationRunnerimplementation instead of timer delay for the animation loop.Animationinterface for hooking up custom animation implementations.CssAnimationimplementation for managing the state lifecycle of css class based animations.ng-if,ng-unless,ng-show,ng-hide,ng-repeat,ng-switch.Notes:
ClockandBiMap<K, V>.