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

[Standalone for NativeComponents] RCTPicker #22996

Closed
wants to merge 4 commits into from
Closed

[Standalone for NativeComponents] RCTPicker #22996

wants to merge 4 commits into from

Conversation

Chi-AnTai
Copy link
Contributor

[iOS] [Changed] - As #22990 said, move requireNativeComponent to a separate file.
I am not familiar with flow, I try to follow the https://pastebin.com/RFpdT76V example but I am not sure I have done it right.

Test Plan:

RNTester success

@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 Jan 15, 2019
analysis-bot
analysis-bot suggested changes Jan 15, 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.
  • flow found some issues.

Libraries/Components/Picker/RCTPickerNativeComponent.js Outdated Show resolved Hide resolved
Libraries/Components/Picker/RCTPickerNativeComponent.js Outdated Show resolved Hide resolved
Libraries/Components/Picker/RCTPickerNativeComponent.js Outdated Show resolved Hide resolved
@pull-bot
Copy link

Warnings
⚠️

📋 Changelog - This PR appears to be missing Changelog.

Generated by 🚫 dangerJS

analysis-bot
analysis-bot suggested changes Jan 15, 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/Components/Picker/PickerIOS.ios.js Outdated Show resolved Hide resolved
@elicwhite
Copy link
Member

elicwhite commented Jan 15, 2019 •
edited
Loading

Thanks for the PR! This looks pretty good. It looks like you have a flow error. You can check out the analyze circle ci job to see the failure.

Edit: looks like you may have fixed that while I was typing this!

@Chi-AnTai
Copy link
Contributor Author

@TheSavior I think I fix the flow error, but I don't understand the test_detox_end_to_end and test_ios error message. Should I try to fix it or it's some bug not related to this PR?

Also, thank you guys for creating react-native, it give me a really good experience when developing with it. I am glad I can do some small effort to help.

@elicwhite elicwhite self-assigned this Jan 16, 2019
facebook-github-bot
facebook-github-bot reviewed Jan 16, 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.

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

facebook-github-bot
facebook-github-bot reviewed Jan 22, 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.

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

@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 Jan 22, 2019
@react-native-bot
Copy link
Collaborator

@Chi-AnTai merged commit 6c18069 into facebook:master.

@facebook facebook locked as resolved and limited conversation to collaborators Jan 22, 2019
@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 22, 2019
@hramos hramos removed Import Started This pull request has been imported. This does not imply the PR has been approved. labels Feb 6, 2019
matt-oakes pushed a commit to matt-oakes/react-native that referenced this pull request Feb 7, 2019
@Chi-AnTai @facebook-github-bot
RCTPicker (facebook#22996)
499da14 
Summary:
[iOS] [Changed] - As facebook#22990 said, move requireNativeComponent to a separate file.
I am not familiar with flow, I try to follow the https://pastebin.com/RFpdT76V example but I am not sure I have done it right.
Pull Request resolved: facebook#22996

Differential Revision: D13697082

Pulled By: cpojer

fbshipit-source-id: c0c87a8e1a7f0553da994aba230f69b496140200
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

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

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

@cpojer cpojer cpojer approved these changes

Assignees

@elicwhite elicwhite

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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@Chi-AnTai @pull-bot @elicwhite @react-native-bot @cpojer @facebook-github-bot @analysis-bot @hramos