[ANDROID] Add snapToInterval support for horizontal ScrollView

[henrikra]

This PR is based on PR #10242

• add implementation of snapToInterval to Android
• modify documentation so snapToInterval is not only iOS prop now
• add example to RNTester

You don't have to add pagingEnabled to the ScrollView so now iOS and Android work the same way. snapToInterval is only added to horizontal scroll because I have not seen any use cases for snapping in vertical scroll.

I also think that later there should be only snapToInterval prop so pagingEnabled could be deprecated. Since pagingEnabled is just snapToInterval with value Dimensions.get('window').width. Any comments or thoughts?

Demo of new behaviour:

[henrikra]

What you guys think? @lelandrichardson @dmmiller @hollanderbart @spikebrehm @brentvatne @drop-dan @hramos

[pull-bot]

	Warnings
⚠️	📋 Test Plan - This PR appears to be missing a Test Plan.

Generated by 🚫 dangerJS

[kevinleeTCA]

Really awesome feature, any idea when this will be accepted? @henrikra

[henrikra]

@kevinleeTCA When someone would review this PR :/ It has been two weeks already

[chrusart]

Travis checks are required to pass, not sure about review process but maybe no one will look at it until they pass?

[henrikra]

It fails randomly. You cant trust it

[chrusart]

yeah, now version in flowconfig is ^0.52.0, they just update it and then flow version somehow is not the same (^ compatible) on building machine i guess.

[kevinleeTCA]

Really nice feature, I got confused that react team did not do it in the first place. This feature can ensure the performance of the interaction on mobile. I tried to do similar customised swipe using panResponder, it is only around 30fps and sluggish.

[henrikra]

@kevinleeTCA Please also try this branch out :D

[TSMMark]

Please merge

[sladkoff]

Can we get a review on this? Is any other action required for this to be merged?

[henrikra]

I could fix conflicts if I know someone would merge this or even test it, otherwise not

[sigvef]

Please do :) There's my alternative implementation ready for merge here: #15667 . I don't mind which one gets merged, as long as we can land this feature!

[facebook-github-bot]

@henrikra I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

[philipjc-apacio]

There is a case for vertical scrolling, when the device orientation is fixed to landscape. Thoughts?

[bd-arc]

@henrikra This is a must-needed addition to the ScrollView component, thanks a lot!

To build up on @philipjc-apacio's comment, there are definitely use cases for vertical scrolling. For example, a carousel like this one could be rewritten to take advantage of FlatList's optimizations instead of being stuck with a custom PanResponder.

Also, do you plan on adding support for snapToAlignment? It would be awesome to provide this feature on both platforms.

[facebook-github-bot]

@Vince0613 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[chrusart]

There would be use cases for vertical, but please finish this PR as a horizontal only, which will resolve most of use cases (probably more than 90% is horizontal). It's not a problem to make a PR for vertical as well later.
Many users wait for this PR to be merged.

[yedidyak]

Is there any news on this?

[Vince0613]

Sorry guys, I didn't mean to pull this and check it in. I was looking to see if it worked as I expected it too. Should I abandon the pull from phabricator?

…

On Wed, Nov 1, 2017 at 5:37 AM Yedidya Kennard ***@***.***> wrote: Is there any news on this? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#15297 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APhGco7Ca99ytj8MuURdO99IvqSMMbGEks5syGXygaJpZM4Oo7FR> .

[ghost]

Hey! I'm willing to do anything at all to help along this PR from July being merged before 2017 is gone forever and 2018 rolls around. Please let me know if there's anything I can do to help.

[vshab]

Seems like one of such PR's has been merged - ddd65f1

[hugoh59]

Hey it's 2018, could we get this to pass all the test and merge? Would be really awesome...

[Ciberusps]

Any progress?
UPD: Oops sorry i skiped @vshab answer. For anyone who works with expo u need to wait new Expo version and upgrade, u can monitor anounce and upgrade here https://blog.expo.io/

[hramos]

This PR has been open for a while, with conflicts that have yet to be addressed. If anyone is interested in supporting this change, please do open a new PR.

[kevinleeTCA]

It seems react team release a new react-native version which support snapToInterval now.
https://github.com/facebook/react-native/releases/tag/v0.53.0

[baldursson]

The current implementation of snapToInterval in Android expects you to enable paging, so the behaviour is very different from iOS which is very disappointing.

[dgreensp]

@baldursson Thanks for pointing that out; I wondered why snapToInterval did not seem to be working for me on Android! Can you elaborate about how the behavior is very different from iOS?

The docs for snapToInterval seem to be quite out of date, incidentally; they don't even say it works on Android. And they certainly don't say you need to set pagingEnabled={true}, in fact they say it overrides pagingEnabled.

[baldursson]

@dgreensp The snapToInterval option is used to create some form of paging, right? When enabled on iOS, you can scroll fast through multiple pages and the view will snap when the scrolling has deaccelerated. However, when pagingEnabled is set, the scrollview will act like a web carousel, only allowing the user to scroll one page at a time. So snapToInterval and pagingEnabled have two completely different use cases and shouldn't be mixed up. Like they have on Android.

[dgreensp]

@baldursson I haven't tried pagingEnabled on iOS, but I compared snapToInterval on iOS with the new snapToInterval+pagingEnabled on Android, with a snapToInterval width of half the screen size. There is a slight difference in behavior in what happens if you "fling" (lift your finger when velocity is significantly different from zero); on Android, it will jump instantly to the next page/interval, while on iOS you get a little bit of momentum, though if you are using decelerationRate=fast then you are only going to scroll through a few pages that way. In non-flinging cases, they behave similarly.

I don't see the completely different use cases with paging vs. interval if an interval is just a variable-width page, and a page is just a fixed-width interval.

In my case, I'm ending up writing my own snapping because I need horizontal and vertical snapping (and also 2D scroll on iOS and Android, not just one dimension at a time, for which nested ScrollViews works quite well).

[baldursson]

@dgreensp Let me elaborate on the different use cases:

I'm making an overview of a digital book with hundreds if pages. So I've used a horizontal scroll view with snapToInterval. That means I can swiftly browse through all these pages, but the scroll view will always snap on a page when scrolling is done. On iOS that is.

The behaviour enabled with pagingEnabled is great when you only have a few pages, typically with page indicators beneath. Like an onboarding screen or image carousel where you normally browse through one page at a time.

So to sum it up:
The snapToInterval behaviour on iOS is great when you have a LOT of pages.
The pagingEnabled behaviour on iOS is great when you only have A FEW pages.

[PimDeWitte]

I have used this to create one for vertical as well. #18679

It's not up to date with the master branch and I currently don't have time to make it work on master. If anyone merge this and wants to add vertical as well, please look at the code I wrote up there and put it in. If I have some time over the next few weeks I'll make a PR into master. If anyone does it in the meantime, plz let me know.

Thanks -)