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 ability to control scroll animation duration for Android #22884

Closed
wants to merge 25 commits into from
Closed

Add ability to control scroll animation duration for Android #22884

wants to merge 25 commits into from

Conversation

osdnk
Copy link
Contributor

@osdnk osdnk commented Jan 6, 2019

Motivation:

This is one of the more sought after feature requests for RN:
react-native.canny.io/feature-requests/p/add-speed-attribute-to-scrollto

This PR adds the support to add a "duration" whenever using "scrollTo" or "scrollToEnd" with
a scrollView. Currently this only exists for Android as the iOS implementation will be somewhat more involved.

This PR is also backwards compatible and does not yet deprecate the "animated" boolean. It may not make sense to ever deprecate "animated", as it could be the flag that is used when devs want the system default duration (which is 250ms for Android). I'm not sure what it is for iOS. It would simplify things to remove "animated", though.

Test Plan:

Create a ScollView component that overflows the bounds of the container, e.g.:

  render() {
    return (
      <View style={styles.container}>
        <Button onPress={this.scrollToPosition} title="Scroll to Position" color="#841584" />
        <ScrollView style={styles.scrollView} ref={scrollView => this.scrollView = scrollView}>
          {items.map(item => {
            return <View key={item.key} style={styles.scrollViewItem}><Text>{item.text}</Text></View>
          })}
        </ScrollView>
      </View>
    );
  }

Using the "scrollView" ref and the Button callback, scroll to a position with a desired duration speed, e.g:

scrollToPosition = () => {
  this.scrollView && this.scrollView.scrollTo({
    y: 500
    duration: 1000
  });
};

Ensure the legacy "animated" boolean works as expected:

scrollToPosition = () => {
  this.scrollView && this.scrollView.scrollTo({
    y: 500
    animated: true
  });
};

Test that you can animate the x position as well (change the ScrollView to be horizontal).
Test that the scroll animation is canceled when touching on the scroll view in the middle of an animation.
Test that repeated calls to animate the scrollView don't queue up (e.g. smashing the in the example above, and then tapping into the scrollView should cancel any animation movement).
Vertical scroll:
drive.google.com/file/d/1VhaolHx63GOv0YWTg7ay0GtzW1nIk635/view?usp=sharing

Horizontal scroll:
drive.google.com/file/d/1yq5P1k4-KPPu9BhS6B9liL-NRGX1Z4gA/view?usp=sharing

Related PRs:

Abandoned PR to bring "duration" to iOS:
#1334

Updated docs on react-native-website:
facebook/react-native-website#115

Changelog:

[Android] [Enhancement] - Added support for controlling animation duration speed for methods "scrollTo" and "scrollToEnd"

This is a copy of #17422 with resolved conflicts and style fixes.

@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 Jan 6, 2019
@pull-bot
Copy link

pull-bot commented Jan 6, 2019

Warnings
⚠️

📋 Changelog - This PR may have incorrectly formatted Changelog. Please format it as explained in the contributing guidelines.

Generated by 🚫 dangerJS

@react-native-bot react-native-bot added the Platform: Android Android applications. label Jan 6, 2019
@osdnk
Copy link
Contributor Author

osdnk commented Jan 12, 2019

Ping @hramos

@hramos
Copy link
Contributor

hramos commented Jan 14, 2019

@osdnk thanks for picking up the work! I think the only remaining piece of feedback left to apply would be @janicduplessis's Apr 24, 2018 comment:

Could you make this so that it uses smoothScrollTo when using the default duration (just {animated: true})? I'd rather keep the default android behavior and this will avoid changing the scrolling behavior in subtle ways.

Looking at the source for smoothScrollTo it seems to do a lot more than just animating the scroll position. See https://android.googlesource.com/platform/frameworks/base/+/jb-release/core/java/android/widget/ScrollView.java#1103 and https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master/core/java/android/widget/OverScroller.java. Sadly there isn't a way to customize the OverScroll instance easily (maybe possible with some crazy reflection stuff).

What do you think?

@osdnk
Copy link
Contributor Author

osdnk commented Jan 14, 2019

I thought it will be a bit easier. I need to dig into this PR deeped

@osdnk
Copy link
Contributor Author

osdnk commented Jan 16, 2019

/@hramos @janicduplessis

Thanks! I simplified it a little bit following my intuition and your advices!
Take a look

Copy link
Contributor

@janicduplessis janicduplessis left a comment

Choose a reason for hiding this comment

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

Code looks good! Just one small thing to make it clearer.

@osdnk
Copy link
Contributor Author

osdnk commented Jan 16, 2019

Here you are @janicduplessis

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

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

Many people have reviewed this PR and it's previous version over the last year and the authors have done everything that we asked for. I am assuming this is safe to land now so I'm going ahead and ship this.

@facebook-github-bot facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Jan 29, 2019
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

@osdnk merged commit 7e8b810 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Jan 29, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 29, 2019
@fkgozali
Copy link
Contributor

Sorry for the churn, but seems like this broke some internal callsites, so we'll have to revert it for now. We'll figure out how to address it and update this thread again -- @cpojer

@hramos hramos added Reverted and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Jan 31, 2019
matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019
…k#22884)

Summary:
Motivation:
----------
This is one of the more sought after feature requests for RN:
react-native.canny.io/feature-requests/p/add-speed-attribute-to-scrollto

This PR adds the support to add a "duration" whenever using "scrollTo" or "scrollToEnd" with
a scrollView. Currently this only exists for Android as the iOS implementation will be somewhat more involved.

This PR is also backwards compatible and does not yet deprecate the "animated" boolean. It may not make sense to ever deprecate "animated", as it could be the flag that is used when devs want the system default duration (which is 250ms for Android). I'm not sure what it is for iOS. It would simplify things to remove "animated", though.
Pull Request resolved: facebook#22884

Differential Revision: D13860038

Pulled By: cpojer

fbshipit-source-id: f06751d063a33d7046241c95348b6abbb327d36f
@satya164
Copy link
Contributor

Damn, I was so excited for this :(

Seems the docs still mention it. I was trying to use it and scratching my head why it doesn't work.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: Android Android applications. Reverted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants