Support flash scroll indicators for android #15566

AlanFoster · 2017-08-20T01:09:44Z

There is missing support for flash scroll indicators on Android. This PR adds this functionality.

Test Plan

Ensured that the functionality works now within iOS and Android

Existing iOS Functionality

New Android Functionality

facebook-github-bot · 2017-08-20T01:09:59Z

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. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

eslint-bot · 2017-08-20T01:18:33Z

Libraries/Components/ScrollResponder.js

@@ -18,6 +18,7 @@ var ReactNative = require('ReactNative');
 var Subscribable = require('Subscribable');
 var TextInputState = require('TextInputState');
 var UIManager = require('UIManager');
+var Platform = require('Platform')


semi: Missing semicolon.

facebook-github-bot · 2017-08-20T01:20:17Z

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

janicduplessis

Thanks for the PR! Just some small changes to make the code simpler.

janicduplessis · 2017-08-20T02:08:12Z

Libraries/Components/ScrollResponder.js

@@ -471,6 +472,15 @@ var ScrollResponderMixin = {
   * @platform ios
   */
  scrollResponderFlashScrollIndicators: function() {
+    if (Platform.OS === 'android') {


We could just get rid of the iOS code path and always use UIManager.dispatchViewManagerCommand, afaik it also works on iOS.

Hmm! In honesty, I wasn't quite sure why on iOS there was an explicit ScrollViewManager as a native module, but on android there isn't one

Unfortunately I'm lacking the iOS-foo to test this change out, and I can't see any equivalent docs for running a modified of react-native with iOS - similar to the the Android Building React Native from source docs. I'm going to have to punt on this one unfortunately!

I think ViewManagers as modules were just never implemented on Android. I was mostly basing this comment on other methods like scrollTo uses only dispatchViewManagerCommand even on iOS. On iOS react nativr is already built from source so you could test by either running the RNTester xcodeproj or changing code under node_modules in an app.

But there should not be any native changes required on iOS, it should just work when using dispatchViewManagerCommand

Ah; There's some magic inside the UIManager.js to handle iOS. Gotcha now 👍

Updated :)

janicduplessis · 2017-08-20T02:09:10Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java

@@ -92,6 +92,10 @@ public void setPagingEnabled(boolean pagingEnabled) {
    mPagingEnabled = pagingEnabled;
  }

+  public boolean flashScrollIndicators() {


What is the boolean returned by this? Since we're not using it might as well not return it.

I've nuked it now! :)

Just for visibility though; The android API is:

true if the animation is played, false otherwise

janicduplessis · 2017-08-20T02:09:39Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactScrollView.java

@@ -121,6 +121,10 @@ public void setScrollEnabled(boolean scrollEnabled) {
    mScrollEnabled = scrollEnabled;
  }

+  public boolean flashScrollIndicators() {


Same for the boolean here

shergin · 2017-08-20T02:27:20Z

...tAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollViewManager.java

@@ -125,6 +125,11 @@ public void receiveCommand(
  }

  @Override
+  public void flashScrollIndicators(ReactHorizontalScrollView scrollView) {
+    scrollView.flashScrollIndicators();


We can call awakenScrollBars right from here, right?

Unfortunately not as it's protected :)

shergin · 2017-08-20T02:27:37Z

ReactAndroid/src/main/java/com/facebook/react/views/scroll/ReactHorizontalScrollView.java

@@ -92,6 +92,10 @@ public void setPagingEnabled(boolean pagingEnabled) {
    mPagingEnabled = pagingEnabled;
  }

+  public boolean flashScrollIndicators() {
+    return super.awakenScrollBars();


Why super?

Just a small left-over piece of code; I was initially overriding to just widen the scope:

@Override public boolean awakenScrollBars() { return super.awakeScrolBars(); }

But I decided to align the names with the existing API; And now the super was out of place. Deleted now, and the PR has been updated :)

AlanFoster · 2017-08-21T22:31:08Z

@janicduplessis All feedback applied now; Is the end result good to ship? :)

janicduplessis · 2017-08-22T02:01:25Z

Thanks @AlanFoster, looks good to me,

@shergin I think this is good to ship.

shergin · 2017-08-22T05:56:26Z

Libraries/Components/ScrollResponder.js

@@ -471,8 +471,11 @@ var ScrollResponderMixin = {
   * @platform ios


We have to remove it now. 🎉

facebook-github-bot · 2017-08-23T06:02:13Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

facebook-github-bot · 2017-08-23T15:27:30Z

@shergin has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

AlanFoster · 2017-08-23T16:46:30Z

Thanks for the PR review guys! 👍

Could I possibly chance my arm for a review of migrating the network inspector from ListView->FlatList? #15548 😇

I'm hoping to put up an additional PR once that's approved to make the Network inspector's scrollToBottom mechanism a lot nicer for developers, but I was hoping to get these pre-requisite PRs in first :)

facebook-github-bot · 2017-08-24T05:48:22Z

I tried to merge this pull request into the Facebook internal repo but some checks failed. To unblock yourself please check the following: Does this pull request pass all open source tests on GitHub? If not please fix those. Does the code still apply cleanly on top of GitHub master? If not can please rebase. In all other cases this means some internal test failed, for example a part of a fb app won't work with this pull request. I've added the Import Failed label to this pull request so it is easy for someone at fb to find the pull request and check what failed. If you don't see anyone comment in a few days feel free to comment mentioning one of the core contributors to the project so they get a notification.

shergin · 2017-08-24T06:06:03Z

Seems I have to fix some internal stuff before it can be landed. I hope I will do it soon.

facebook-github-bot added the GH Review: review-needed label Aug 20, 2017

eslint-bot reviewed Aug 20, 2017

View reviewed changes

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 Aug 20, 2017

AlanFoster force-pushed the support-flash-scroll-indicators-for-android branch from ea6626a to c43950e Compare August 20, 2017 01:24

janicduplessis suggested changes Aug 20, 2017

View reviewed changes

shergin reviewed Aug 20, 2017

View reviewed changes

AlanFoster force-pushed the support-flash-scroll-indicators-for-android branch 2 times, most recently from 5d63ed8 to 48f4b68 Compare August 20, 2017 23:29

Support flash scroll indicators for android

4d0bd13

AlanFoster force-pushed the support-flash-scroll-indicators-for-android branch from 48f4b68 to 4d0bd13 Compare August 20, 2017 23:30

janicduplessis approved these changes Aug 22, 2017

View reviewed changes

shergin suggested changes Aug 22, 2017

View reviewed changes

shergin added 2 commits August 22, 2017 23:00

Update ScrollResponder.js

a92e8f7

Merge branch 'master' into support-flash-scroll-indicators-for-android

07de1d3

facebook-github-bot added GH Review: accepted Import Started This pull request has been imported. This does not imply the PR has been approved. and removed GH Review: review-needed labels Aug 23, 2017

Merge branch 'master' into support-flash-scroll-indicators-for-android

df52db8

facebook-github-bot added Import Failed and removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Aug 24, 2017

shergin self-assigned this Aug 24, 2017

facebook-github-bot closed this in 2ceed95 Aug 28, 2017

AlanFoster mentioned this pull request Sep 18, 2017

Update Network Overlay to support smarter forced scrolling mechanism #15993

Closed

AlanFoster deleted the support-flash-scroll-indicators-for-android branch September 18, 2017 21:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support flash scroll indicators for android #15566

Support flash scroll indicators for android #15566

AlanFoster commented Aug 20, 2017

facebook-github-bot commented Aug 20, 2017

eslint-bot Aug 20, 2017

facebook-github-bot commented Aug 20, 2017

janicduplessis left a comment

janicduplessis Aug 20, 2017

AlanFoster Aug 20, 2017

janicduplessis Aug 20, 2017

janicduplessis Aug 20, 2017

janicduplessis Aug 20, 2017

AlanFoster Aug 20, 2017

janicduplessis Aug 20, 2017

AlanFoster Aug 20, 2017

janicduplessis Aug 20, 2017

shergin Aug 20, 2017

AlanFoster Aug 20, 2017

shergin Aug 20, 2017

AlanFoster Aug 20, 2017

AlanFoster commented Aug 21, 2017 •

edited

Loading

janicduplessis commented Aug 22, 2017

shergin Aug 22, 2017

facebook-github-bot commented Aug 23, 2017

facebook-github-bot commented Aug 23, 2017

AlanFoster commented Aug 23, 2017

facebook-github-bot commented Aug 24, 2017

shergin commented Aug 24, 2017

		@@ -471,8 +471,11 @@ var ScrollResponderMixin = {
		* @platform ios

Support flash scroll indicators for android #15566

Support flash scroll indicators for android #15566

Conversation

AlanFoster commented Aug 20, 2017

Test Plan

Existing iOS Functionality

New Android Functionality

facebook-github-bot commented Aug 20, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Aug 20, 2017

janicduplessis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlanFoster commented Aug 21, 2017 • edited Loading

janicduplessis commented Aug 22, 2017

Choose a reason for hiding this comment

facebook-github-bot commented Aug 23, 2017

facebook-github-bot commented Aug 23, 2017

AlanFoster commented Aug 23, 2017

facebook-github-bot commented Aug 24, 2017

shergin commented Aug 24, 2017

AlanFoster commented Aug 21, 2017 •

edited

Loading