Skip to content

Add scrollEnabled example to RNTester#23409

Closed
berickson1 wants to merge 4 commits intofacebook:masterfrom
berickson1:patch-3
Closed

Add scrollEnabled example to RNTester#23409
berickson1 wants to merge 4 commits intofacebook:masterfrom
berickson1:patch-3

Conversation

@berickson1
Copy link
Copy Markdown
Contributor

Summary

RNTester is missing a test for the scrollEnabled prop

Changelog

[General] [Added] - Added RNTester ScrollView scrollEnabled prop test

Test Plan

Tested in RNTester app

@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. p: Microsoft Partner: Microsoft Partner labels Feb 12, 2019
Copy link
Copy Markdown

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.
  • flow found some issues.

Comment thread RNTester/js/ScrollViewExample.js Outdated
Comment thread RNTester/js/ScrollViewExample.js Outdated
Comment thread RNTester/js/ScrollViewExample.js Outdated
Comment thread RNTester/js/ScrollViewExample.js Outdated
Comment thread RNTester/js/ScrollViewExample.js Outdated
Copy link
Copy Markdown

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

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

Code analysis results:

  • eslint found some issues.

Comment thread RNTester/js/ScrollViewExample.js Outdated
Copy link
Copy Markdown
Contributor

@hramos hramos left a comment

Choose a reason for hiding this comment

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

Thanks for increasing our test coverage! Run prettier to fix the remaining nits. I'll wait for CI to finish running before merging.

@hramos hramos self-assigned this Feb 12, 2019
@cpojer
Copy link
Copy Markdown
Contributor

cpojer commented Feb 12, 2019

Can you run prettier using yarn lint --fix on your project and push again? Thank you!

@berickson1
Copy link
Copy Markdown
Contributor Author

Snapshot tests appear to be failing on iOS since I added the new example - can anyone explain how to update the snapshot images?

@hramos
Copy link
Copy Markdown
Contributor

hramos commented Feb 12, 2019

  1. Open RNTester/RNTester.xcodeproj in Xcode, then open RNTesterSnapshotTests.m. Change L29 to _runner.recordMode = YES;. This will make it so that snapshots are re-recorded on the next run.
  2. Select the iPhone 6S simulator from the list of devices in the Xcode toolbar (the simulator used here should match the IOS_DEVICE envvar in scripts/.tests.env).
  3. You can then hit CMD+U (Menubar > Product > Test) to run tests.
  4. Afterwards, switch recordMode back to NO.

You should end up with a new set of snapshots for the ScrollViewExample test.

@berickson1
Copy link
Copy Markdown
Contributor Author

berickson1 commented Feb 12, 2019

Perfect, thanks update snapshots as well.
It is confusing that there also seems to be a bunch of outdated snapshot pngs in the folder structure

@hramos
Copy link
Copy Markdown
Contributor

hramos commented Feb 13, 2019

We should look into that. We're only using the iOS12 snapshots on Circle CI, but I'm not sure if our internal test suite relies on the iOS 10 and iOS 11 snapshots for additional tests.

All looks good on this PR, merging! (The detox failure was fixed in master an hour or so ago)

@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 Feb 13, 2019
Copy link
Copy Markdown
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.

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

@react-native-bot
Copy link
Copy Markdown
Collaborator

@berickson1 merged commit 08a6b57 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 13, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Feb 13, 2019
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 14, 2019
rozele pushed a commit to microsoft/react-native-windows that referenced this pull request Jul 18, 2019
Summary:
RNTester is missing a test for the scrollEnabled prop

[General] [Added] - Added RNTester ScrollView scrollEnabled prop test
Pull Request resolved: facebook/react-native#23409

Differential Revision: D14059824

Pulled By: hramos

fbshipit-source-id: 0287277b64aeac69c4aeba83dbb3f73be646ede7
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. p: Microsoft Partner: Microsoft Partner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants