Replace ScrollView.scrollTo() API with something less ambiguous.

nicklockwood · facebook-github-bot-4 · commit 6941c4e027ab · 2016-02-03T04:00:40.000-08:00
Summary:
public
The current `ScrollView.scrollTo()` API is confusing due to the `(y, x)` parameter order, and the boolean `animated` argument. E.g.

    ScrollView.scrollTo(5, 0, true) // what do these arguments mean?

This diff replaces the API with a configuration object, so the arguments are all explicit:

    ScrollView.scrollTo({x: 0, y: 5, animated: true}) // much better

The `scrollTo()` method checks the argument types, and provides backwards compatibility with the old argument format for now. Using the old API will generate a warning, and this will eventually be upgraded to an error.

Reviewed By: davidaurelio

Differential Revision: D2892287

fb-gh-sync-id: cec4d504242391267c6e863816b6180ced7a7d5e
diff --git a/Examples/UIExplorer/ScrollViewExample.js b/Examples/UIExplorer/ScrollViewExample.js
@@ -19,6 +19,8 @@ var React = require('react-native');
 var {
   ScrollView,
   StyleSheet,
+  Text,
+  TouchableOpacity,
   View,
   Image
 } = React;
@@ -31,27 +33,45 @@ exports.examples = [
   title: '<ScrollView>',
   description: 'To make content scrollable, wrap it within a <ScrollView> component',
   render: function() {
+    var _scrollView: ScrollView;
     return (
-      <ScrollView
-        automaticallyAdjustContentInsets={false}
-        onScroll={() => { console.log('onScroll!'); }}
-        scrollEventThrottle={200}
-        style={styles.scrollView}>
-        {THUMBS.map(createThumbRow)}
-      </ScrollView>
+      <View>
+        <ScrollView
+          ref={(scrollView) => { _scrollView = scrollView; }}
+          automaticallyAdjustContentInsets={false}
+          onScroll={() => { console.log('onScroll!'); }}
+          scrollEventThrottle={200}
+          style={styles.scrollView}>
+          {THUMBS.map(createThumbRow)}
+        </ScrollView>
+        <TouchableOpacity
+          style={styles.button}
+          onPress={() => { _scrollView.scrollTo({y: 0}); }}>
+          <Text>Scroll to top</Text>
+        </TouchableOpacity>
+      </View>
     );
   }
 }, {
   title: '<ScrollView> (horizontal = true)',
   description: 'You can display <ScrollView>\'s child components horizontally rather than vertically',
   render: function() {
+    var _scrollView: ScrollView;
     return (
-      <ScrollView
-        automaticallyAdjustContentInsets={false}
-        horizontal={true}
-        style={[styles.scrollView, styles.horizontalScrollView]}>
-        {THUMBS.map(createThumbRow)}
-      </ScrollView>
+      <View>
+        <ScrollView
+          ref={(scrollView) => { _scrollView = scrollView; }}
+          automaticallyAdjustContentInsets={false}
+          horizontal={true}
+          style={[styles.scrollView, styles.horizontalScrollView]}>
+          {THUMBS.map(createThumbRow)}
+        </ScrollView>
+        <TouchableOpacity
+          style={styles.button}
+          onPress={() => { _scrollView.scrollTo({x: 0}); }}>
+          <Text>Scroll to start</Text>
+        </TouchableOpacity>
+      </View>
     );
   }
 }];
diff --git a/Libraries/Components/ScrollResponder.js b/Libraries/Components/ScrollResponder.js
@@ -349,14 +349,29 @@ var ScrollResponderMixin = {
 
   /**
    * A helper function to scroll to a specific point  in the scrollview.
-   * This is currently used to help focus on child textviews, but this
-   * can also be used to quickly scroll to any element we want to focus
+   * This is currently used to help focus on child textviews, but can also
+   * be used to quickly scroll to any element we want to focus. Syntax:
+   *
+   * scrollResponderScrollTo(options: {x: number = 0; y: number = 0; animated: boolean = true})
+   *
+   * Note: The weird argument signature is due to the fact that, for historical reasons,
+   * the function also accepts separate arguments as as alternative to the options object.
+   * This is deprecated due to ambiguity (y before x), and SHOULD NOT BE USED.
    */
-  scrollResponderScrollTo: function(offsetX: number, offsetY: number, animated: boolean = true) {
+  scrollResponderScrollTo: function(
+    x?: number | { x?: number; y?: number; animated?: boolean },
+    y?: number,
+    animated?: boolean
+  ) {
+    if (typeof x === 'number') {
+      console.warn('`scrollResponderScrollTo(x, y, animated)` is deprecated. Use `scrollResponderScrollTo({x: 5, y: 5, animated: true})` instead.');
+    } else {
+      ({x, y, animated} = x || {});
+    }
     UIManager.dispatchViewManagerCommand(
       React.findNodeHandle(this),
       UIManager.RCTScrollView.Commands.scrollTo,
-      [offsetX, offsetY, animated],
+      [x || 0, y || 0, animated !== false],
     );
   },
 
@@ -369,15 +384,24 @@ var ScrollResponderMixin = {
   },
 
   /**
-   * A helper function to zoom to a specific rect in the scrollview.
-   * @param {object} rect Should have shape {x, y, width, height}
-   * @param {bool} animated Specify whether zoom is instant or animated
+   * A helper function to zoom to a specific rect in the scrollview. The argument has the shape
+   * {x: number; y: number; width: number; height: number; animated: boolean = true}
+   *
+   * @platform ios
    */
-  scrollResponderZoomTo: function(rect: { x: number; y: number; width: number; height: number; }, animated: boolean = true) {
+  scrollResponderZoomTo: function(
+    rect: { x: number; y: number; width: number; height: number; animated?: boolean },
+    animated?: boolean // deprecated, put this inside the rect argument instead
+  ) {
     if (Platform.OS === 'android') {
       invariant('zoomToRect is not implemented');
     } else {
-      ScrollViewManager.zoomToRect(React.findNodeHandle(this), rect, animated);
+      if ('animated' in rect) {
+        var { animated, ...rect } = rect;
+      } else if (typeof animated !== 'undefined') {
+        console.warn('`scrollResponderZoomTo` `animated` argument is deprecated. Use `options.animated` instead');
+      }
+      ScrollViewManager.zoomToRect(React.findNodeHandle(this), rect, animated !== false);
     }
   },
 
diff --git a/Libraries/Components/ScrollView/ScrollView.js b/Libraries/Components/ScrollView/ScrollView.js
@@ -359,17 +359,36 @@ var ScrollView = React.createClass({
     return React.findNodeHandle(this.refs[INNERVIEW]);
   },
 
-  scrollTo: function(destY: number = 0, destX: number = 0, animated: boolean = true) {
+  /**
+   * Scrolls to a given x, y offset, either immediately or with a smooth animation.
+   * Syntax:
+   *
+   * scrollTo(options: {x: number = 0; y: number = 0; animated: boolean = true})
+   *
+   * Note: The weird argument signature is due to the fact that, for historical reasons,
+   * the function also accepts separate arguments as as alternative to the options object.
+   * This is deprecated due to ambiguity (y before x), and SHOULD NOT BE USED.
+   */
+  scrollTo: function(
+    y?: number | { x?: number, y?: number, animated?: boolean },
+    x?: number,
+    animated?: boolean
+  ) {
+    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 || {});
+    }
     // $FlowFixMe - Don't know how to pass Mixin correctly. Postpone for now
-    this.getScrollResponder().scrollResponderScrollTo(destX, destY, animated);
+    this.getScrollResponder().scrollResponderScrollTo({x: x || 0, y: y || 0, animated: animated !== false});
   },
 
   /**
    * Deprecated, do not use.
    */
-  scrollWithoutAnimationTo: function(destY: number = 0, destX: number = 0) {
+  scrollWithoutAnimationTo: function(y: number = 0, x: number = 0) {
     console.warn('`scrollWithoutAnimationTo` is deprecated. Use `scrollTo` instead');
-    this.scrollTo(destX, destY, false);
+    this.scrollTo({x, y, animated: false});
   },
 
   handleScroll: function(e: Object) {
