UI Pack: Option to skip property resetting

[rosslavery]

I'm wrapping Velocity / Velocity UI into an AngularJS module at the moment, and to do so I'm running 'In' transitions, and then automatically prepping the element to run the corresponding "Out" animation when appropriate.

The issue comes when Velocity UI goes to run the completed callbacks after all effect calls have been run. In situations as described above, it actually needs to skip over the code block that runs between lines 518 and 526 if (effect.reset) evaluates to true.

Would it be possible to add an option (skipReset?) to optionally bypass that code block?

I can create a PR, I just need to know if you'd like the default value of false defined in Velocity's defaults. This would be a UI-Pack-only option so I'm not sure if it's appropriate to define it's default value in the parent library or not.

Thoughts?

[julianshapiro]

Hey Ross,

First off, your implementation is almost perfect -- only tweak is that it can be shortened slightly.

Second, I'm intrigued by your proposal, but I unfortunately do not understand the problem you're solving. Could you please elucidate and/or create a JSFiddle example for me to play with?

Thank you so much. Super appreciated.

[rosslavery]

Hey Julian,

I've made a plnkr that demonstrates the issue: http://plnkr.co/edit/Rr85oTDPspLLPDEhV4UE?p=preview

You can see that as you click the toggle button, the DOM begins to leak and get duplicated, when there should only ever be 1 element present depending on the condition of the isShowing boolean.

[tomByrer]

Skipping animation resets are also desirable if there is already an animation running, and you (likely) want to continue from where you are presently in the current-previous animation.

[rosslavery]

Hey Julian,

Took a deep look into this just now. It appears to be a timing issue with the complete callback.

I've done a small refactor (+rebased to newest release) to how the complete callback is defined that avoids needing to wrap it in an outer function definition and then use .call() from inside it.

This seems like a win-win to me -- no more skipReset option needed, and the timing issues related to the complete callback + ngAnimate appear to be resolved.

Thoughts?

Edit: Just noticed the changes I've made will run the reset effect too early, rather than after the last animation call is done...hmm.

[rosslavery]

Having just read that you're planning on releasing some major updates over the coming days/weeks, I'm going to close those for now.

Appreciate the help so far.

[julianshapiro]

Please try working with the updated UI pack. (Just pushed.) Let me know how this issue concludes. I'd love to talk further about publicizing this plugin. Thanks a bunch, @rosslavery.

[rosslavery]

Hey Julian,

Still no dice. The issue only occurs when I run the "opposite" animation of the one that helped it enter the DOM.

Angular's animation system requires that you pass in an animation to run for when a specified DOM element is added, moved (re-ordered), and removed.

What this means is that if someone chooses to use a slideDownIn for when their element is added, the true opposite animation should run on it's removal - so it appears to return to it's origin. This means that slideUpOut is the expected motion, rather than slideDownOut.

Regardless of this, it doesn't seem to matter since both of those Out animations have reset properties which runs the code-block that is causing the issue with Angular's callback (if (properties.reset) {}).

Another solution I can think of to avoid adding the questionable skipReset option would be to expose the default UI pack animations (var effects -> Container.Velocity.defaultEffects ?) for users to modify/extend.

At the moment they are a private variable inside of the UI pack, but if they were exposed for the user to modify and extend, it would allow me to iterate through the defaults, remove the reset property for my use-case, and register them with the new RegisterUI method.

This has an upside for all users of being able to use the default animations as a base that they can extend upon, rather than having to copy/paste the entire effect to re-register it with RegisterUI.

[rosslavery]

By the way - just to confirm I'm not crazy :D, I tossed in https://github.com/cgwyllie/angular-velocity and unfortunately the same issue occurs with that library.

[rosslavery]

For my own needs of getting my application live, I ended up just copy-pasting the full effects list from the UI pack and then doing:

  angular.forEach(effects, function(animationProps, animationName) {
    animationProps.reset && delete animationProps.reset;
    $.Velocity.RegisterUI(animationName, animationProps);
  });

The downside obviously being I'll need to do this whenever a cool new effect is added :/

Look forward to figuring out a proper solution together though!

[julianshapiro]

Another solution I can think of to avoid adding the questionable skipReset option would be to expose the default UI pack animations (var effects -> Container.Velocity.defaultEffects ?) for users to modify/extend.

I have an idea of how I can provide you with access to the packaged effects JSON without polluting the Velocity namespace. Let me run it and make sure it doesn't cause errors anywhere. Then I'll chew on it for a night. Expect a concrete update tomorrow.

[rosslavery]

Hey Julian,

Just as a follow up from bed, I spent more time working on this. It appears
to be a scoping issue, not sure on which end.

Basically, I send the call to velocity WITHOUT providing a complete
callback. Instead, I use a setTimeout equal to the duration of the
animation and run it from within the context of Angular, rather than within
Velocity's closure opts.complete that gets created if the animation has a
reset.

Doing the above, it works perfectly.

That may well be a good enough solution since the issue seems quite Angular - specific. I wouldn't want you to mangle your codebase in any way to accommodate this strange issue.

Just wanted to keep you updated on my progress.
On Jul 3, 2014 10:43 PM, "Julian Shapiro" [email protected] wrote:

Another solution I can think of to avoid adding the questionable skipReset
option would be to expose the default UI pack animations (var effects ->
Container.Velocity.defaultEffects ?) for users to modify/extend.

I have an idea of how I can provide you with access to the packaged
effects JSON without polluting the Velocity namespace. Let me run it and
make sure it doesn't cause any errors anywhere. Then I'll chew on it for a
night.

—
Reply to this email directly or view it on GitHub
#132 (comment)
.

[julianshapiro]

Ahh interesting. I'm in the middle of carrying out your good suggestion of externalizing the packaged effects data for optional re-registration. I still think I'm going to do it regardless.

Thank you so much for the update, Ross.

[julianshapiro]

OK. I just pushed it :) You can now access $.Velocity.RegisterUI.packagedEffects.

Btw, in your plugin, make sure you follow the Container pattern seen in velocity.ui.js. Can't remember if you were doing that or not.