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.

Sign up for GitHub

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

Jump to bottom

Support snapToInterval for horizontal scrollview on Android #10242

Closed
wants to merge 1 commit into from
Closed

Support snapToInterval for horizontal scrollview on Android #10242

wants to merge 1 commit into from

Conversation

pp026pp
Copy link

@pp026pp pp026pp commented Oct 4, 2016

snapToInterval is available on iOS but on android yet. This PR is to add support for snapToInterval on android.

Example:

android_snap

TO: @lelandrichardson @spikebrehm

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at [email protected]. Thanks!

@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 Oct 4, 2016
@lelandrichardson lelandrichardson added Android and removed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Oct 4, 2016
@facebook-github-bot
Copy link
Contributor

@pp026pp updated the pull request - view changes

@pp026pp pp026pp force-pushed the jimmyz/snapToInterval_on_android branch from a354080 to 44459b6 Compare October 4, 2016 18:44
@facebook-github-bot
Copy link
Contributor

@pp026pp updated the pull request - view changes

@lelandrichardson
Copy link
Contributor

@pp026pp does this only work for the horizontal scrollview? we might want to add support for the verticle scrollview as well.

Also, it would be nice to update the docs to have snapToInterval not an ios-only prop now.

@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 Oct 4, 2016
@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

dmmiller
dmmiller reviewed Oct 4, 2016
View reviewed changes
Copy link

@dmmiller dmmiller left a comment

Choose a reason for hiding this comment

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

We should also make this work on both horizontal and normal scrollview.

Libraries/Components/ScrollView/ScrollView.js
@@ -497,6 +498,10 @@ const ScrollView = React.createClass({
props.decelerationRate = processDecelerationRate(decelerationRate);
}

if (Platform.OS === 'android' && props.snapToInterval) {
Copy link

Choose a reason for hiding this comment

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

This shouldn't happen here. It should be adjusted in native code like we do all other measures

@facebook-github-bot
Copy link
Contributor

@pp026pp updated the pull request - view changes

@pp026pp pp026pp force-pushed the jimmyz/snapToInterval_on_android branch from 6af6c72 to 277dde4 Compare October 5, 2016 05:51
@facebook-github-bot
Copy link
Contributor

@pp026pp updated the pull request - view changes

@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 Oct 5, 2016
eslint-bot
eslint-bot reviewed Oct 5, 2016
View reviewed changes
Libraries/Components/ScrollView/ScrollView.js
@@ -12,6 +12,7 @@
'use strict';

const ColorPropType = require('ColorPropType');
const Dimensions = require('Dimensions');
Copy link

Choose a reason for hiding this comment

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

no-unused-vars: 'Dimensions' is defined but never used

@pp026pp
Copy link
Author

pp026pp commented Oct 5, 2016

@dmmiller @lelandrichardson Thanks for the review! Looks like pagingEnabled is not supported for android vertical scroll view yet. I feel we probably want to support that first before adding snapToInterval for android vertical scroll view. Do you think it makes sense to only including the snapToInterval change for horizontal scroll in this PR?

@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 Oct 5, 2016
@pp026pp pp026pp force-pushed the jimmyz/snapToInterval_on_android branch from 277dde4 to c894826 Compare October 5, 2016 06:08
@facebook-github-bot
Copy link
Contributor

@pp026pp updated the pull request - view changes

@pp026pp pp026pp changed the title Support snapToInterval on Android Support snapToInterval for horizontal scrollview on Android Oct 5, 2016
@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 Oct 5, 2016
@lelandrichardson
Copy link
Contributor

@pp026pp seems reasonable... we'd be in a strictly better place than we were in before

@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 Oct 5, 2016
@pp026pp pp026pp force-pushed the jimmyz/snapToInterval_on_android branch from c894826 to 0610854 Compare October 5, 2016 23:22
@facebook-github-bot
Copy link
Contributor

@pp026pp updated the pull request - view changes

@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 Oct 5, 2016
@lelandrichardson
Copy link
Contributor

@pp026pp can you rebase again? I think you're stuck on a broken sha

@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 Oct 7, 2016
@lacker lacker removed the Size/S label Oct 25, 2016
@hollanderbart
Copy link
Contributor

+1

@spikebrehm
Copy link

@pp026pp can you rebase?

@brentvatne
Copy link
Collaborator

@pp026pp - rebase please :O

@pp026pp pp026pp force-pushed the jimmyz/snapToInterval_on_android branch from 0610854 to 0c44323 Compare November 5, 2016 19:19
@oblador oblador mentioned this pull request Nov 27, 2016
Closed
@hramos hramos closed this Dec 22, 2016
@drop-dan
Copy link

Sorry to comment on an old, closed pull-request. Was this only closed because the maintainer disappeared? Would love to finish this off, especially if it was just a rebase that prevented merging

@hramos
Copy link
Contributor

hramos commented Mar 10, 2017

It was closed due to non-response for almost two months. Feel free to open a new PR.

@henrikra henrikra mentioned this pull request Jul 31, 2017
Closed
facebook-github-bot pushed a commit that referenced this pull request Jan 3, 2018
@facebook-github-bot
Support snapToInterval for horizontal scrollview on Android
ddd65f1 
Summary:
`snapToInterval` is available on iOS but on android yet. This PR is to add support for `snapToInterval` on android.

Example:

![android_snap](https://cloud.githubusercontent.com/assets/1699429/19086983/39d3ee1c-8a25-11e6-9c84-20f25a751f32.gif)

TO: lelandrichardson spikebrehm
Closes #10242

Differential Revision: D4168527

fbshipit-source-id: de3dd9ac5d9e0fddfce5e5bc0aa6a4f33f1e30b3
vincentriemer pushed a commit to vincentriemer/react-native-dom that referenced this pull request Jan 9, 2018
@facebook-github-bot
Support snapToInterval for horizontal scrollview on Android
e608ffb 
Summary:
`snapToInterval` is available on iOS but on android yet. This PR is to add support for `snapToInterval` on android.

Example:

![android_snap](https://cloud.githubusercontent.com/assets/1699429/19086983/39d3ee1c-8a25-11e6-9c84-20f25a751f32.gif)

TO: lelandrichardson spikebrehm
Closes facebook/react-native#10242

Differential Revision: D4168527

fbshipit-source-id: de3dd9ac5d9e0fddfce5e5bc0aa6a4f33f1e30b3
mathiasbynens pushed a commit to mathiasbynens/react-native that referenced this pull request Jan 13, 2018
@mathiasbynens
Support snapToInterval for horizontal scrollview on Android
98ea141 
Summary:
`snapToInterval` is available on iOS but on android yet. This PR is to add support for `snapToInterval` on android.

Example:

![android_snap](https://cloud.githubusercontent.com/assets/1699429/19086983/39d3ee1c-8a25-11e6-9c84-20f25a751f32.gif)

TO: lelandrichardson spikebrehm
Closes facebook#10242

Differential Revision: D4168527

fbshipit-source-id: de3dd9ac5d9e0fddfce5e5bc0aa6a4f33f1e30b3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmmiller dmmiller dmmiller left review comments

@eslint-bot eslint-bot eslint-bot left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.