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

Add support for scrollToTransition prop #28

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

gabrielecirulli
Copy link
Contributor

@gabrielecirulli gabrielecirulli commented Oct 24, 2017

As outlined in #27, we're working on an interactive timeline. It would benefit from being able to animate when the user clicks on it and the scrollToIndex prop changes.

This adds a prop called scrollToTransition which, when passed, uses the FLIP Technique to perform a high-performance transition between the original position and the one after the new scroll position is set. The result is a change that's much easier for the user to understand as, rather than jumping, the list animates between two locations.

The prop uses the usual CSS transition syntax, but requires that you leave out the name of the property, which is then forced to transform.


Issues:

  • Race condition when performing too many scrollToIndex prop changes at the same time
    • Basic fix (check for presence of transition style)
    • Better fix? (e.g. cancel the transition and run a new one, or debounce the reset function?)
  • Items disappear during the animation when moving a large distance across the list, due to the virtual list not rendering items that aren't in view.

Used in combination with `scrollToIndex`, if passed this prop adds a
transition when `scrollToIndex` changes. Use the same syntax as the CSS
`transition` property. Do not specify a property. Example transitions:
`200ms ease`, `1s cubic-bezier(0.4, 0.0, 0.2, 1)`
@gabrielecirulli
Copy link
Contributor Author

I have to mention that due to time constraints I didn't write tests for this feature or test it on other use cases than our horizontal timeline (i.e. a vertical list). If you think this change is worth offering to the users of this project, let me know. I'd appreciate your help testing it and I might be able to dedicate some time to it too if necessary.

@gabrielecirulli
Copy link
Contributor Author

There is also a race condition caused by triggering multiple scrollToIndex changes while the animation is still running which breaks all further animations. Looking into that.

@gabrielecirulli
Copy link
Contributor Author

Added a basic fix on this. Any better ideas on how it could be fixed?

In general this PR might need some more work. Another issue is that due to instancing/deinstancing of items that are out of view, when the transition starts it'll look like the list is empty, and you won't see any items other than the ones you'd see at the end of the animation.

Again, I'm first of all interested in hearing if this feature is worthwhile , but I could help with these improvements if so.

@gabrielecirulli
Copy link
Contributor Author

when the transition starts it'll look like the list is empty, and you won't see any items other than the ones you'd see at the end of the animation.

A solution to this particular issue would be to render the visible ranges around the start and target items, plus the items in-between during the transition.

@gabrielecirulli
Copy link
Contributor Author

@clauderic any opinions on this?

@gabrielecirulli
Copy link
Contributor Author

@clauderic any thoughts? We may soon be able to help bring this a little bit further if there's interest (we'll still need this feature so we may end up having to use a fork of this project)

@RXminuS
Copy link

RXminuS commented Feb 3, 2018

Would love to see this merged into master as well. Awesome job @gabrielecirulli . Will have to use a fork as well.

@rssfrncs
Copy link

Any update on this? Would be a feature i'd definitely use!

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.

3 participants