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.

Sign up for GitHub

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

Jump to bottom

[TM] add spec for PlatformConstants #24928

Closed
wants to merge 14 commits into from
Closed

[TM] add spec for PlatformConstants #24928

wants to merge 14 commits into from

Conversation

sattaman
Copy link
Contributor

Summary

part of #24875.

Changelog

[General] [Added] - add TM spec for PlatformConstants

Test Plan

Tests + flow pass

@facebook-github-bot
Copy link
Contributor

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!

@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label May 17, 2019
@facebook-github-bot 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 May 17, 2019
@facebook-github-bot
Copy link
Contributor

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

@sattaman
Update yarn.lock
0ce9e48
@sattaman
Update Platform.android.js
8d102b2 
Update import of platform contants into Platform.android
analysis-bot
analysis-bot suggested changes May 17, 2019
View reviewed changes
Copy link

@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. Run yarn lint --fix to automatically fix problems.

Libraries/Utilities/Platform.android.js Show resolved Hide resolved
@sattaman
Copy link
Contributor Author

I didn't realise when picking this up @jarvisluong It looks like it might it might have been better if I'd picked up the ios part of PlatformConstants too since its all the same module .

@jarvisluong
Copy link

@sattaman right, that is fine too!

analysis-bot
analysis-bot suggested changes May 17, 2019
View reviewed changes
Copy link

@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:

  • flow found some issues.

Libraries/Utilities/Platform.ios.js Show resolved Hide resolved
Libraries/Utilities/Platform.ios.js Show resolved Hide resolved
RNTester/js/TouchableExample.js Outdated Show resolved Hide resolved
@sattaman
Update ReactNativeVersionCheck.js
a295ea4 
Updated equality check to simplify
@sattaman
Update NativePlatformConstants.js
1e8cfe6 
Switch export depending on platform
@sattaman
Update Platform.ios.js
9664e82 
fix typo
@sattaman
Update TouchableExample.js
ffe1985 
revert changes to TouchableExample
@sattaman
Update NativePlatformConstants.js
d1dc1f7 
Update the name of Android module for PlatformConstants
@sattaman
Update setup.js
bfd359c 
add mock for NativePlatformContants
fkgozali
fkgozali reviewed May 21, 2019
View reviewed changes
Copy link
Contributor

@fkgozali fkgozali left a comment

Choose a reason for hiding this comment

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

otherwise looks good

yarn.lock Outdated Show resolved Hide resolved
Libraries/Utilities/NativePlatformConstants.js Outdated Show resolved Hide resolved
Libraries/Utilities/NativePlatformConstants.js Outdated Show resolved Hide resolved
fkgozali
fkgozali requested changes May 21, 2019
View reviewed changes
Copy link
Contributor

@fkgozali fkgozali left a comment

Choose a reason for hiding this comment

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

back to your queue

@sattaman
Copy link
Contributor Author

@fkgozali I've updated as requested

facebook-github-bot
facebook-github-bot reviewed May 30, 2019
View reviewed changes
Copy link
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.

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

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @sattaman in 7fd08e1.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label May 30, 2019
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
@sattaman @M-i-k-e-l
add spec for PlatformConstants (facebook#24928)
0fc8cf1 
Summary:
part of facebook#24875.

## Changelog

[General] [Added] - add TM spec for PlatformConstants
Pull Request resolved: facebook#24928

Reviewed By: RSNara

Differential Revision: D15551340

Pulled By: fkgozali

fbshipit-source-id: 9de15ff4cfe717f963332868bd873d5147a37506
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@RSNara RSNara RSNara left review comments

@uqmessias uqmessias uqmessias left review comments

@fkgozali fkgozali fkgozali requested changes

@analysis-bot analysis-bot analysis-bot requested changes

@wojteg1337 wojteg1337 wojteg1337 approved these changes

Assignees
No one assigned
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. Native Module Type: Enhancement A new feature or enhancement of an existing feature.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@sattaman @facebook-github-bot @jarvisluong @react-native-bot @fkgozali @uqmessias @RSNara @wojteg1337 @analysis-bot @hramos