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

Added wait() method to delay movement at the specified point #30

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

klapstoelpiloot
Copy link

This implements my request #29

Copy link
Owner

@mobius3 mobius3 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I see the point of having a wait() feature. There are a couple things that needs to be addressed in your PR:

  • your indentation looks off in regard to the rest of the project (it may be that the indentation of the project looks off in regard to the rest of the world but it is what it is :)
  • I se that you've made the calculateTotal() function. Since it's not returning a calculation but rather updating totals, I believe updateStackedTotals() would be a better name orsomething else that implies change
  • I'd implement wait() using tween::during() instead of duplicating it's code. I'd also say to call to() before, with the last value, but unboxing it may be somewhat hard to do so (see tween::dispatch(), not pretty). But for the duration, calling tween::during() is the way to go. And maybe we don't actually need the calculateTotal/updateStackedtotals function anymore.
  • I believe you should set the stepped easing for this. By default tweeny it uses linear easing which makes unecessary calculations in this case. stepped returns always the first value which is what you want here.
  • You should explain in the docs of this function that it is effectively equal to calling (from(a).to(a).during(...).via(stepped)) because although we're providing a useful functionality, we're also breaking user expectations in regards to the amount of easing points. It is known that to() adds a point but that wait() adds another is not and it's not obvious and this breaks seek() and jump() expectations. This has to be made explicit.

Thanks again for your PR!

@klapstoelpiloot
Copy link
Author

Fair points. I'll see what I can do to implement your requested changes.

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.

2 participants