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

UI Pack: Incorrect handling of defaults values #239

Closed
jods4 opened this issue Aug 14, 2014 · 2 comments
Closed

UI Pack: Incorrect handling of defaults values #239

jods4 opened this issue Aug 14, 2014 · 2 comments

Comments

@jods4
Copy link

jods4 commented Aug 14, 2014

Velocity consistently uses the || operator to set default values, like this: opt.easing = param.easing || 'ease';.
This is a bad JS habit, because it doesn't work when the value passed is falsy but not undefined (e.g. false or 0).

I got biten by this bug here: https://github.com/julianshapiro/velocity/blob/master/velocity.ui.js#L83
opts.queue = sequenceOptions.queue || "";
This is wrong because the documentation says that passing the option queue: false executes the animation immediately. The code above in the UI pack silently ignores a passed false option.

A suggested fix is to use $.extend to set default values. Or in this case -- because you are white-listing the options you copy -- the longer opts.queue = sequenceOptions.queue !== undefined ? sequenceOptions.queue : "";

BTW I stumbled on two related bugs.
(1) The UI pack doesn't white-list (i.e. copy) the option _cacheValues any reason for that? It prevents using transitions on an element that you may have modified directly.

(2) It is highly annoying that these options don't work: { stagger: 75, queue: false }. The problem seems to be that not queuing bypasses everything in the queue including the delays created by the animate call itself! It would be a lot more useful if queue: false bypasses everything else, with the exception of its own stuff!

@julianshapiro
Copy link
Owner

  • Relax with your hyperbole. I don't owe you a library that works just the way you'd like it to.
  • As for the queue:false issue, it has to be that way otherwise multi-call UI pack effects will have each individual call run at the same time. I will update the docs now to reflect that these effects cannot be used with queue: false. Thank you for pointing this out.
  • No reason for not passing through _cacheValues. I will do so now. Expect this change in the next update. Thank you.
  • Not sure what you're referring to exactly in the last bug. I think you may mistakenly be thinking that stagger works with non-UI pack calls (it doesn't, as the docs state).

@julianshapiro julianshapiro changed the title Incorrect handling of defaults values UI Pack: Incorrect handling of defaults values Aug 14, 2014
@jods4
Copy link
Author

jods4 commented Aug 14, 2014

"Relax with your hyperbole" Sorry about that and not really sure what you mean, because English is not my native language. :(

queue: false: I didn't thinnk about that, it wouldn't work with the current implementation indeed. API-wise it's a bit unintuitive and disappointing that you can't start a transition with this option, but I don't see an easy way to fix it.

You could chain the calls through the complete event rather than the queue, and only dequeue or call next on the queue when the last step is done. That would be rather complicated for a small benefit. I worked around that by calling stop before the transition.

The last "bug" is related to the first. I fixed my copy of the library to pass-through queue: false, but then stagger didn't work anymore. I can see clearly now that's because all the calls were called with queue: false and they all started immediately.

Thinking as I write, maybe there is a clean way to fix those things: if you put a "Sequence" or "sub-queue" in the jQuery queue. That is: an object that is itself a queue of calls and which only dequeues when it is empty.

Rycochet pushed a commit that referenced this issue Aug 3, 2020
Fixes CommonJS module loading. Closes #232. Closes #238.

Allow animating with the raw utility function (Velocity) instead of its
property (Velocity.animate), e.g. $.Velocity(element, propertiesMap,
options);

Copy over _cacheValues option for the UI pack. Closes #239.

Reverts Velocity.mock to original (asynchronous) behavior. Closes #237.

Fixes IE7 error throwing in UI pack. Closes #246.
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

No branches or pull requests

2 participants