-
Notifications
You must be signed in to change notification settings - Fork 24.5k
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
[Standalone for NativeComponents] AndroidViewPagers.js #22995
Conversation
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 [email protected]. Thanks! |
Generated by 🚫 dangerJS |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@TheSavior, sorry for the ping but I'm not quite able to comprehend the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the iOS errors are unrelated.
Thanks for moving this out into its own file! One of the important parts of this issue is to define the flow type for this component. Can you help do that for this PR? You can probably figure out most of the type by looking at the file that originally included the call site and the props passed to it.
For the syntax of how to do this you can take a look at SwitchNativeComponent.js
Thanks for the clarification. On it! (^_^) |
There was a problem hiding this 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.
All the imports connected to requireNativeComponent in ViewPager was moved to a seperate file.
54ffc6d
to
aa7e02a
Compare
There was a problem hiding this 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.
e649341
to
943bbc7
Compare
Added missing flow type for native component to document.
@TheSavior, Sorry for the delay, I was having minor trouble with implementing the flow type for the component. I have run into the problem of inexact types and have tried various ways to counter it without any success [ci/circleci: analyze]. I have not made any changes to the part throwing this error and am having a hard time finding a solution. Would love some direction to solve this (^_^) |
Nevermind, I found the error 😅. The |
There was a problem hiding this 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.
Thanks for the PR! While your change is technically breaking, I think you are changing the callback to be consistent so it makes sense to me to change. |
I observed the call to be inconsistent with the rest and found a hard time writing the flow type. When I cross referenced with the other calls, the parameters always seemed to be an event and hence decided to change it. I was waiting for a review to verify whether that decision was right or wrong. Thanks for the approval (^_^) |
@VisibleMarkov merged commit ec488dc into |
Summary: [Android][Changed] - All the imports connected to `requireNativeComponent` in `ViewPager` was moved to a separate file. Issue in focus: facebook#22990 Pull Request resolved: facebook#22995 Differential Revision: D13760459 Pulled By: cpojer fbshipit-source-id: fca1633ce933ea4909ef81d7bbe8123d654e24fb
Summary: [Android][Changed] - All the imports connected to `requireNativeComponent` in `ViewPager` was moved to a separate file. Issue in focus: #22990 Pull Request resolved: facebook/react-native#22995 Differential Revision: D13760459 Pulled By: cpojer fbshipit-source-id: fca1633ce933ea4909ef81d7bbe8123d654e24fb
Summary: [Android][Changed] - All the imports connected to `requireNativeComponent` in `ViewPager` was moved to a separate file. Issue in focus: #22990 Pull Request resolved: facebook/react-native#22995 Differential Revision: D13760459 Pulled By: cpojer fbshipit-source-id: fca1633ce933ea4909ef81d7bbe8123d654e24fb
[Android][Changed] - All the imports connected to
requireNativeComponent
inViewPager
was moved to a separate file.Issue in focus: #22990
Test Plan: