Added animations for show/hide #20
Conversation
|
|
|
@dabide hey there, thanks for the contribution. As far as I can see, the only necessary file to be included in this PR should be |
|
Better now? |
|
However, in light of this, I should probably switch to the CSS animator... :-( julianshapiro/velocity#769 |
|
Yeah thats great ... now I haven't actually worked on the velocity implementation of this plugin, I was active on the CSS animator. Nevertheless I'll try to do a proper review of your changes. If you've some time and will you're welcome to add some unit tests as well ;) |
| @@ -351,6 +377,8 @@ export class VelocityAnimator { | |||
| el.animations = {}; | |||
| el.animations.enter = this._parseAttributeValue(el.getAttribute('anim-enter')) || this.enterAnimation; | |||
| el.animations.leave = this._parseAttributeValue(el.getAttribute('anim-leave')) || this.leaveAnimation; | |||
| el.animations.show = this._parseAttributeValue(el.getAttribute('anim-show')) || undefined; | |||
There was a problem hiding this comment.
if the animation for leave/show is not defined shouldn't we use defaults like for enter/leave? See line 22/23
There was a problem hiding this comment.
I initially did that, but reconsidered. Then enter/leave effects are opt-in by adding au-animate. If hide/show should have a default animation, I think we would also have to add a check for that attribute. (Or perhaps accept empty anim-leave and anim-show attributes.)
| return this.stop(element, true)._runElementAnimation(element, ':hide', undefined, 'hide').then(() => { | ||
| element.classList.add(className); | ||
| }); | ||
| } else { |
There was a problem hiding this comment.
the else block is not really needed since the if contains a return
| @@ -250,8 +258,14 @@ export class VelocityAnimator { | |||
| * @returns Resolved when the animation is done | |||
| */ | |||
| addClass(element: HTMLElement, className: string): Promise<boolean> { | |||
There was a problem hiding this comment.
I see that enter/leave do accept an options attribute, is that not needed for remove/add?
There was a problem hiding this comment.
No, it is not used when templating calls enter/leave.
| @@ -239,8 +242,13 @@ export class VelocityAnimator { | |||
| * @returns Resolved when the animation is done | |||
| */ | |||
| removeClass(element: HTMLElement, className: string): Promise<boolean> { | |||
There was a problem hiding this comment.
shouldn't the function return type be Promise<boolean|VelocityAnimator> ?
There was a problem hiding this comment.
I'm no TypeScript expert. However, _runElementAnimation() returns Promise<any>, so then it could just as well be Promise<any> for removeClass(). But wouldn't that violate the Animator interface definition, which has Promise<bool>? Should I perhaps add a .then() that returns a boolean on success?
There was a problem hiding this comment.
The then approach is the way to go 👍
There was a problem hiding this comment.
Just make sure to add a catch as well in case something goes wrong
|
@zewa666 Where are we at on this? |
|
Looks good I think we can merge it |
|
Can we remove the dependency on templating/resources here? It seems it's only for the sake of a single string. |
|
Well since the hide-class name might change we should keep it the way it is. I wouldn't be a too big fan of hard-coding it. |
|
Because of one string we're creating a hard dependency between this library and the other one that doesn't make sense otherwise. Additionally, I'm not sure the internal reference to the module will work correctly with all loader scenarios. If we really want this to be a dependency, then we should move the const into the templating library so that both resources and animation can reference the same thing. |
|
But since that const is exported, moving it might causes breaking changes for consumers. Both approaches not very satisfying but in light of that I'd rather hardcode the value. I'll go forward and adapt the PR |
Added support for
anim-showandanim-hideattributes, and thusshow.bindanimations.