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.

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

fix for removed View.propTypes #286

Merged
merged 4 commits into from
Mar 13, 2018
Merged

fix for removed View.propTypes #286

merged 4 commits into from
Mar 13, 2018

Conversation

yusukeshib
Copy link
Contributor

Fixed the issue: #279

@holden-caulfield
Copy link

Please merge this... so far the library can't be use on a production release that uses the latest RN because of this bug

@iwmitt
Copy link

iwmitt commented Dec 17, 2017

Could you merge this already? The whole library is unusable because this simple fix isn't merged.

@brunoczo
Copy link

I've wasted hours looking for the problem. Please fix this. To solve local, i edit the node_module.

@flippinjoe
Copy link

Sweet Jesus. Push the button

@mattiamanzati
Copy link

+1

Copy link

@habovh habovh left a comment

Choose a reason for hiding this comment

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

Implementing these changes would assure the library remains compatible with older versions of React Native.
This could ensure fast approval?

index.ios.js Outdated
@@ -90,7 +91,7 @@ class FBLogin extends Component {
}

FBLogin.propTypes = {
style: View.propTypes.style,
style: ViewPropTypes.style,
Copy link

Choose a reason for hiding this comment

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

May I suggest to use a ternary here to keep the library backwards compatible (< RN 0.49)?

Like this:

const viewPropTypes = ViewPropTypes || View.propTypes
FBLogin.propTypes = {
    [...],
    style: viewPropTypes.style,
    [...],
}

@habovh
Copy link

habovh commented Jan 16, 2018

@YusukeShibata it seems you also forgot to update their example component at example/components/facebook/FBLoginMock.js, but that's not preventing the component itself from being used.

@kayzenkayzen
Copy link

Please merge!!

@nachocifu
Copy link

+1

@yusukeshib
Copy link
Contributor Author

Thanks @habovh , I pushed new commits.

@Dalamar
Copy link

Dalamar commented Jan 25, 2018

Please make it finally available in the master branch.
I can't release my app.
Worth to mention that it took me 2 days to figure out why does my app crashes on released versions.

@habovh
Copy link

habovh commented Jan 25, 2018

@Dalamar If you're in a hurry you can still use @YusukeShibata's fork from GitHub instead of the official npm package until it gets merged.

@magus magus merged commit 8333768 into magus:master Mar 13, 2018
@magus
Copy link
Owner

magus commented Mar 13, 2018

Thanks for reporting this, the fix is out in version 1.6.1!

magus added a commit that referenced this pull request Oct 14, 2018
magus added a commit that referenced this pull request Oct 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet