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 support for flashScrollIndicators on iOS #14058

Closed

Conversation

jeanregisser
Copy link
Contributor

Motivation

Flashing scroll indicators is a standard behavior on iOS to show the user there's more content.

Test Plan

Launch RNTester on iOS, go to the ScrollView section, tap the "Flash scroll indicators" button.
You'll see this:

Flash scroll indicators

Misc

I've exposed the method flashScrollIndicators on all scrolling components that were already exposing a scrollToXXX method so it's usable from those components using a ref.

Let me know what you think.

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. GH Review: review-needed labels May 19, 2017
* @platform ios
*/
flashScrollIndicators: function() {
this.getScrollResponder().scrollResponderFlashScrollIndicators()

Choose a reason for hiding this comment

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

semi: Missing semicolon.

@facebook-github-bot 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 May 22, 2017
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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.

@facebook-github-bot 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 May 22, 2017
@jeanregisser
Copy link
Contributor Author

@javache let me know if there's anything I can do to make the import succeed.

@javache
Copy link
Member

javache commented May 24, 2017

Can you update the ScrollViewExample snapshot test? This is currently breaking tests.

@jeanregisser
Copy link
Contributor Author

@javache done!

Note: while generating the updated snapshots, I noticed 2 issues:

Is it still worth it to update non -iOS10 snapshots? Are automated tests only running for iOS10?
Should I open new PR to fix these?

Thanks

@shergin shergin self-requested a review May 26, 2017 23:25
@shergin shergin added the Platform: iOS iOS applications. label May 26, 2017
@shergin
Copy link
Contributor

shergin commented May 26, 2017

I am afraid, we have to redo snapshots one more time... because I changed them.

@jeanregisser
Copy link
Contributor Author

jeanregisser commented May 29, 2017

@shergin what iOS versions did you use to generate the snapshots?

I rebased my changes against the latest master and tried to regenerate the snapshots, but I got more changed snapshots than testScrollViewExample_1*. I used iOS 10.1 and 10.2. and got the same change for both on testSliderExample_1-iOS10@2x.png.

Here is a visual diff: left is original version in master you generated, middle is diff (differences are in red), right is my generated version:

diff

As you can see, the difference is only in the last 2 examples that use a custom track image.
Looking at slider.png and slider-left.png or slider-right.png I see that they have an embedded color profile: Generic RGB Profile.

I'm wondering if the new snapshot infra is dependant on a specific screen color profile.
What's your take on this?

@jeanregisser
Copy link
Contributor Author

jeanregisser commented May 29, 2017

I just tried changing my screen color profile and got the same results as in my initial test.

Also I can see that the example with the thumbs up image uie_thumb_big.png is not affected by the difference. And this one has no color profile embedded.

@shergin
Copy link
Contributor

shergin commented May 29, 2017

Oooops, as I mentioned in 3df537a, I changed the way how we make screenshots, and it probably causes your problem. The tests works fine in cloud thought.
I used iPhone 5S and iOS 10.3.

So, should we remove color profile from images?

@jeanregisser
Copy link
Contributor Author

Ok I'll try tomorrow with 10.3
What about the non iOS10 snapshots? Did you use iOS 9.3? Or else?

For iOS10 snapshots, I used iOS 10.3 on iPhone 5S and Apple TV 10.2 simulators.
For non iOS10 snapshots, I used iOS 9 on iPhone 5S and Apple TV 9.2 simulators.

No other snapshots were changed with this setup.
@jeanregisser
Copy link
Contributor Author

Alright @shergin @javache, I got the right output!

For iOS10 snapshots, I used iOS 10.3 on iPhone 5S and Apple TV 10.2 simulators.
For non iOS10 snapshots, I used iOS 9 on iPhone 5S and Apple TV 9.2 simulators.
With these, only the testScrollViewExample_1* were changed, which is expected.

We should probably document what devices and iOS versions are used to generate those snapshots. I had different results using 10.2 or iOS 9.3

@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 May 30, 2017
@facebook-github-bot
Copy link
Contributor

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

@shergin shergin added this to Uncategorized in Better <ScrollView> Jun 1, 2017
@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

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

sayar pushed a commit to microsoft/react-native-windows that referenced this pull request Jul 26, 2017
Summary:
Flashing scroll indicators is a standard behavior on iOS to show the user there's more content.

Launch RNTester on iOS, go to the ScrollView section, tap the "Flash scroll indicators" button.
You'll see this:

![Flash scroll indicators](https://cloud.githubusercontent.com/assets/57791/26250919/ebea607a-3cab-11e7-96c6-27579cc809ab.gif)

I've exposed the method `flashScrollIndicators` on all scrolling components that were already exposing a `scrollToXXX` method so it's usable from those components using a ref.

Let me know what you think.
Closes facebook/react-native#14058

Differential Revision: D5103239

Pulled By: shergin

fbshipit-source-id: caad8474fbe475065418d771b17e4ea9766ffcdc
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Import Started This pull request has been imported. This does not imply the PR has been approved. Platform: iOS iOS applications.
Projects
No open projects
Better <ScrollView>
Uncategorized
Development

Successfully merging this pull request may close these issues.

None yet

5 participants