-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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 #17422
Changes from 4 commits
64e5e7c
e27720b
3df7093
f153d4f
25b5ec6
5399cb4
4bf0dce
7d37961
2896cf1
8dcc2e9
cd49a31
2f54dfc
6161dd8
1eb2cf6
26bc7bc
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -526,29 +526,33 @@ const ScrollView = createReactClass({ | |
}, | ||
|
||
/** | ||
* Scrolls to a given x, y offset, either immediately or with a smooth animation. | ||
* Scrolls to a given x, y offset, either immediately, with a smooth animation, or, | ||
* for Android only, a custom animation duration time. | ||
* | ||
* Example: | ||
* | ||
* `scrollTo({x: 0, y: 0, animated: true})` | ||
* `scrollTo({x: 0, y: 0, animated: true, duration: 0})` | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not a good example because it's unclear what this should do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added a separate example just for duration. |
||
* | ||
* Note: The weird function signature is due to the fact that, for historical reasons, | ||
* the function also accepts separate arguments as an alternative to the options object. | ||
* This is deprecated due to ambiguity (y before x), and SHOULD NOT BE USED. | ||
* | ||
* Also note "duration" is currently only supported for Android. | ||
*/ | ||
scrollTo: function( | ||
y?: number | { x?: number, y?: number, animated?: boolean }, | ||
y?: number | { x?: number, y?: number, animated?: boolean, duration?: number }, | ||
x?: number, | ||
animated?: boolean | ||
animated?: boolean, | ||
duration?: number | ||
) { | ||
if (typeof y === 'number') { | ||
console.warn('`scrollTo(y, x, animated)` is deprecated. Use `scrollTo({x: 5, y: 5, ' + | ||
'animated: true})` instead.'); | ||
} else { | ||
({x, y, animated} = y || {}); | ||
({x, y, animated, duration} = y || {}); | ||
} | ||
this.getScrollResponder().scrollResponderScrollTo( | ||
{x: x || 0, y: y || 0, animated: animated !== false} | ||
{x: x || 0, y: y || 0, animated: animated !== false, duration: duration || 0} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. duration || 0 seems wrong - should probably just send the null. I could imagine people explicitly setting 0 as an equivalent to animated: false. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was following the existing pattern with this API where a default value is always provided. In ReactScrollViewCommandHelper.java, a duration of zero is effectively ignored. It's tricky, though, because we want to support the existing animated API so this is backwards compatible. So what happens if a user says "animated: true, duration: 0", or "animated: false, duration: 250"? In ReactScrollViewCommandHelper, I defer first to duration value. If duration is non-zero, we animate with that duration. Otherwise, if animated is true, we use the legacy animation duration (250). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, thought I replied earlier. I'm not too concerned about cases where users give ambiguous input. If a user says "animated: true, duration: 0" there is no right answer, so we can do whatever - undefined behavior. If the user calls There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I see, I was missing the part where animated defaults to true in this code, and we'd probably want to preserve that, but then not respect it if a user passes duration: 0. |
||
); | ||
}, | ||
|
||
|
@@ -558,15 +562,19 @@ const ScrollView = createReactClass({ | |
* | ||
* Use `scrollToEnd({animated: true})` for smooth animated scrolling, | ||
* `scrollToEnd({animated: false})` for immediate scrolling. | ||
* For Android, you may specify a duration, e.g. `scrollToEnd({duration: 500})` | ||
* for a controlled duration scroll. | ||
* If no options are passed, `animated` defaults to true. | ||
*/ | ||
scrollToEnd: function( | ||
options?: { animated?: boolean }, | ||
options?: { animated?: boolean, duration?: number }, | ||
) { | ||
// Default to true | ||
const animated = (options && options.animated) !== false; | ||
const duration = options ? options.duration || 0 : 0; | ||
this.getScrollResponder().scrollResponderScrollToEnd({ | ||
animated: animated, | ||
duration: duration | ||
}); | ||
}, | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,10 @@ | |
|
||
package com.facebook.react.views.scroll; | ||
|
||
import android.animation.ObjectAnimator; | ||
import android.animation.PropertyValuesHolder; | ||
|
||
import android.view.MotionEvent; | ||
import android.view.View; | ||
import android.view.ViewGroup; | ||
|
||
|
@@ -96,4 +100,28 @@ public static int parseOverScrollMode(String jsOverScrollMode) { | |
throw new JSApplicationIllegalArgumentException("wrong overScrollMode: " + jsOverScrollMode); | ||
} | ||
} | ||
|
||
/** | ||
* Helper method for animating to a ScrollView position with a given duration, | ||
* instead of using "smoothScrollTo", which does not expose a duration argument. | ||
*/ | ||
public static ObjectAnimator animateScroll(final ViewGroup scrollView, int mDestX, int mDestY, int mDuration) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no idea if this Animator object is legit - @janicduplessis or @mdvacca? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It has support from API 11: https://developer.android.com/reference/android/animation/ObjectAnimator.html FWIW we've been using this ScrollView (via overwriting the ScrollView package in MainApplication.Java) in production for the past 6 weeks, in production / release. We also use it in a fairly expensive SectionList which does all sorts of crazy optimizations and is a good stress test for performance. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would still like signoff from someone that knows our android codebase...@janicduplessis, @mdvacca, or @axe-fb? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sounds good. |
||
PropertyValuesHolder scrollX = PropertyValuesHolder.ofInt("scrollX", mDestX); | ||
PropertyValuesHolder scrollY = PropertyValuesHolder.ofInt("scrollY", mDestY); | ||
|
||
final ObjectAnimator animator = ObjectAnimator.ofPropertyValuesHolder(scrollView, scrollX, scrollY); | ||
|
||
// Cancel the animation if a user interacts with the ScrollView. | ||
scrollView.setOnTouchListener(new View.OnTouchListener() { | ||
@Override | ||
public boolean onTouch(View v, MotionEvent event) { | ||
scrollView.setOnTouchListener(null); | ||
animator.cancel(); | ||
return false; | ||
} | ||
}); | ||
|
||
animator.setDuration(mDuration).start(); | ||
return animator; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you open a PR that applies these same changes to https://github.com/facebook/react-native-website/blob/master/docs/scrollview.md ?
These comments are not synced. At some point we need to update these comments with links to the generated docs at http://facebook.github.io/react-native instead (I've done this with a few source files already but had not gotten to ScrollView yet).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done:
facebook/react-native-website#115