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

Carousel: add ability to disable scrolling animation #222

Closed
pixelzoom opened this issue Jan 28, 2016 · 10 comments
Closed

Carousel: add ability to disable scrolling animation #222

pixelzoom opened this issue Jan 28, 2016 · 10 comments
Assignees

Comments

@pixelzoom
Copy link
Contributor

In function-builder, I need to programmatically scroll the carousel in order to determine the location of each item in the carousel in the global coordinate frame. Because of the animation that takes place when you call Carousel.scrollToItem or scrollToItemIndex, I was getting the incorrect result. So this is one situation in which disabling animation is useful, and it seems like a generally useful feature.

@pixelzoom
Copy link
Contributor Author

Feature implemented. Assigning to @jonathanolson to review, since we discussed this briefly today. Rather than adding an animationEnabled flag to scrollToItem and scrollToItem, I added option animationEnabled and setter/getter for animationEnabled field. This allows you to enable/disable animation not only for the scroll functions, but for the Carousel in general.

@pixelzoom pixelzoom assigned jonathanolson and unassigned pixelzoom Jan 28, 2016
@jonathanolson
Copy link
Contributor

Looks good, although one question (unlikely to come up in practice):

I saw scrollTween && scrollTween.stop() was only run if animation was enabled. If the client:

  1. Sets animationEnabled to true
  2. Calls scrollToItem to kick off animation (to item A)
    and before the animation completes:
  3. Sets animationEnabled to false
  4. Calls scrollToItem to something on another page (item B)

Presumably the last scrollToItem wouldn't interrupt the tween, so would the tween cause the scroll to end up at item A while the index is at item B?

@pixelzoom
Copy link
Contributor Author

Good point. Should probably call scrollTween && scrollTween.stop() regardless of whether animationEnabled.

@pixelzoom
Copy link
Contributor Author

Done. @jonathanolson anything else?

@pixelzoom pixelzoom assigned jonathanolson and unassigned pixelzoom Jan 29, 2016
@jonathanolson
Copy link
Contributor

Nope, thanks!

@pixelzoom
Copy link
Contributor Author

Reopening. There's a bug in this feature, it only works for horizontal carousels.

@pixelzoom pixelzoom reopened this Feb 10, 2016
pixelzoom added a commit that referenced this issue Feb 10, 2016
@pixelzoom
Copy link
Contributor Author

Fixed and closing.

@pixelzoom
Copy link
Contributor Author

Reopening.

To reset a carousel without scrolling, the client has to currently do something like this:

var saveAnimationEnabled = myCarousel.animationEnabled;
myCarousel.animationEnabled = false;
myCarousel.reset();
myCarousel.animationEnabled = saveAnimationEnabled;

In Function Builder, I have 3 carousels to reset, so this gets quite ugly. What I'd really like to do is tell reset to temporarily disable animation, and let it handle saving and restoring the animation state, e.g.:

myCarousel.reset( { animationEnabled: false } );

@pixelzoom
Copy link
Contributor Author

Done, closing. If anyone wants to review, go for it.

veillette added a commit that referenced this issue Jul 1, 2016
@veillette
Copy link
Contributor

I assigned this commit to the wrong ticket

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

3 participants