-
Notifications
You must be signed in to change notification settings - Fork 166
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
feat: wrap initialize sdk function #2
feat: wrap initialize sdk function #2
Conversation
I looks fine, but I believe it would be good to have some note in the |
Yes! I believe this would stop the crash I saw and worked around by adding an AppDelegate chunk. +1 on a README note as otherwise I would have no idea, maybe even a major version bump |
README is added, any suggestions if it's not clear. |
@mikehardy I see, that's why you've added the code in |
Besides comments, looks like there is some error in iOS build. @mikehardy I agree with you. this could be a major bump. |
cdf41ff
to
375e7e7
Compare
375e7e7
to
4622827
Compare
The In this pr I will be focused on sdk initialization. |
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 believe now everything looks good to me :)
just accidentally unassigned myself, don't mine it |
Fixed :) |
Let's just wait for @mikehardy feedback on it and I'm good to merge and release 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.
I have no problems with the code (it's easy code thankfully!) but I made a suggestion to restructure the document with the idea it will nudge users to be platform-neutral and use the Javascript code. And I expanded the information about GDPR vs non-GDPR flow as I think that is the real decision point, with most having to handle GDPR style flows (voluntarily or by law) so I paid special attention to directing people how to do that so it is the same on both platforms using the platform-neutral javascript call
Words are easy to smush around though, do anything you like (or nothing at all) with the suggestion :-)
🎉 This PR is included in version 4.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
As you see, release done :) |
Thank you very much @rnike for your PR and the commitment into it that was wonderful and @mikehardy for reviewing and providing valuable feedback on it as well. |
RN62 Compatibility
From issue facebookarchive/react-native-fbsdk#840
I wrap the initialize function from native, the original pr is facebookarchive/react-native-fbsdk#841