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 pagingEnabled for vertical ScrollView on Android #10999

Closed
wants to merge 9 commits into from

Conversation

raymondchooi
Copy link

Summary:
This adds support for pagingEnabled to the Vertical ScrollView on Android.

This is an implementation of a3146e4 for vertical ScrollView. All the changes are pretty similar apart from a few structure changes to accommodate FB's own version of fling for when more content is added while ScrollView is animating. If pagingEnabled={true}, this is not used as smoothScrollToPage() is used instead.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @astreet and @janicduplessis to be potential reviewers.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 17, 2016
* when scrolling. This can be used for horizontal pagination. The default
* value is false.
* when scrolling. This can be used for horizontal pagination. Vertical
* pagination is supported on Android. The default value is false.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think better to just remove the line about horizontal pagination since it works for both now.

@satya164
Copy link
Contributor

cc @andreicoman11

@raymondchooi
Copy link
Author

Ah thanks, I assumed it wasn't available on ios as well as platform wasn't specified. I've tested on ios now and can confirm it works.

@mkonicek
Copy link
Contributor

mkonicek commented Nov 22, 2016

I don't think I've seen vertical paging in an Android app. Is this a common pattern?

@raymondchooi
Copy link
Author

I don't think it's that common but I have seen it in a few, most notably Snapchat. I liked the mental model it built for the user and ease of navigation from gestures. It seemed perfect for a desktop companion app (working on the name..) that I'm currently building.

@aldigjo
Copy link

aldigjo commented Dec 6, 2016

Would very much like to see this merged, snapchat swipe navigation is very nice and would be nice to use a similar navigation with react-native.

@gaguirre
Copy link

Any news on this? @satya164
Would be great to have this in the next release ;)

@satya164
Copy link
Contributor

@mkonicek what do you think? I think it's useful to have and provides feature parity with iOS

@EladPlayground
Copy link

Would like to have this merge as well 👍
The use case for vertical paging in my application is for implementing a typeform (www.typeform.com) style registration form.

@aldigjo
Copy link

aldigjo commented Jan 6, 2017

Really would prefer to not have to wait another month for this.

@satya164
Copy link
Contributor

satya164 commented Jan 6, 2017

@raymondchooi can you rebase the PR against master?

@pelle
Copy link

pelle commented Jan 11, 2017

This is getting increasingly urgent for us. Please can we get it in 0.41

@satya164
Copy link
Contributor

@pelle it's unlikely to go in 0.41. you should use a fork with this commit for now.

@raymondchooi
Copy link
Author

@satya164 My apologies, I haven't used rebase before. I thought I just had to update from master. I've followed some guides and hopefully I've done it correctly.

@mkonicek
Copy link
Contributor

This is a pretty big change in a very frequently used component. This pull request is missing a test plan (see https://github.com/facebook/react-native/blob/master/.github/PULL_REQUEST_TEMPLATE.md). How did you test this doesn't break anything?

Anyone who's blocked on this - consider releasing this as a separate native module.

@facebook-github-bot
Copy link
Contributor

@raymondchooi 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.

@Luc45
Copy link

Luc45 commented Mar 9, 2018

Sorry, I'm learning React Native now. PagingEnabled for Android isn't working?

@react-native-bot react-native-bot added Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Ran Commands One of our bots successfully processed a command. Android labels Mar 16, 2018
@react-native-bot react-native-bot added Platform: Android Android applications. Ran Commands One of our bots successfully processed a command. labels Mar 18, 2018
@josiahruddell
Copy link

Any progress on merging this?

@hramos
Copy link
Contributor

hramos commented May 14, 2018

@josiahruddell I don't see any comments from @raymondchooi after I last commented here. It also looks like we're waiting on a Test Plan to be added to the PR as well.

@raymondchooi
Copy link
Author

Hey, sorry, things have gotten really busy on my end. I updated for merge conflicts a few times before but did not have time to get in to a thorough test plan. If anyone would like to contribute please feel free, else I'll try get to it when I can.

I was also under the impression that this was still under discussion on whether or not this should be in the core. If it is decided, I will try and find some time to add the test plan.

Personally, I think it should be as there are a few layouts such as gesture heavy navigation where this would be useful and it is available for iOS/does not seem OS specific.

@react-native-bot react-native-bot added the Missing Test Plan This PR appears to be missing a test plan. label May 15, 2018
@geolabZura
Copy link

hello, how can i use this native code in my react native application?? this feature so important for my application

@rbravo
Copy link

rbravo commented Jun 18, 2018

For anyone needing this vertical paging feature on iOS and Android with urgency (as I did) you can use the react-native-snap-carousel component for vertical and horizontal slides. It works on both platforms and can be a workaround while this issue is in discussion.

@geolabZura
Copy link

@rbravo i will try it, thanks!!!!

@henninghall
Copy link

Like @rbravo said react-native-snap-carousel might work but it has some other limitations, like all heights has to be equal. Thats not the case with ScrollView/FlatList.

@khat33b
Copy link

khat33b commented Aug 31, 2018

It's been almost 10 months since this pull request was made. When will this be merged? Can a timeline be made available?

@hramos
Copy link
Contributor

hramos commented Sep 7, 2018

@khat33b I already left my feedback earlier, and the author mentioned they didn't have time to work on this. The length of time a PR has been open is not a good metric for whether it should be merged or not.

If anyone wants to contribute a similar fix and has the time to work on getting it landed, you can open a new PR.

Copy link
Contributor

@hramos hramos left a comment

Choose a reason for hiding this comment

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

Add a test plan and release notes, and fix conflicts.

@raymondchooi
Copy link
Author

raymondchooi commented Sep 7, 2018

I can possibly make some time if there's a strong possibility of merging.

I was under the impression that there was still a discussion going on on whether or not this should be in the core RN. If there is a strong possibility of this going in, I'll try make some time :)

@hramos
Copy link
Contributor

hramos commented Sep 12, 2018

I just caught up with the earlier comments. I'll close this PR to make it clear it's unlikely to get merged. If anybody is interested in this functionality, your best bet is to implement it as a separate third party component.

@hramos hramos closed this Sep 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Missing Changelog This PR appears to be missing a changelog, or they are incorrectly formatted. Missing Test Plan This PR appears to be missing a test plan. Platform: Android Android applications. Ran Commands One of our bots successfully processed a command.
Projects
None yet
Development

Successfully merging this pull request may close these issues.