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

Build two versions: one with renderProps only, and one with CSS styling. #196

Merged
merged 3 commits into from Feb 1, 2018

Conversation

Projects
None yet
2 participants
@jackfranklin
Contributor

jackfranklin commented Jan 11, 2018

This is a bit of a meaty PR but it's something we needed at work and I've made the changes we need. I thought I'd propose merging it back in in case you're interested in it.

If you're not, no worries, and we'll use our fork internally and I won't badger you with more PRs!

This PR creates two versions of the component:

  • one that supports only rendering via a render prop
  • another that parcels up the default CSS and renders the built in button styling

The default one that is imported is the one that includes styling, but a user could also import FB from 'react-facebook-login/facebook-login-render-props' and get a version where they have full control of the rendering. The benefit of doing this is that the bundle is around 5kb less in size because there's no CSS needed.

@jackfranklin

This comment has been minimized.

Show comment
Hide comment
@jackfranklin

jackfranklin Jan 11, 2018

Contributor

PS: if you're not using this component so much any more or would like some help maintaining it please do give me a ping as we're using it at thread.com and would love to help :)

Contributor

jackfranklin commented Jan 11, 2018

PS: if you're not using this component so much any more or would like some help maintaining it please do give me a ping as we're using it at thread.com and would love to help :)

@keppelen

This comment has been minimized.

Show comment
Hide comment
@keppelen

keppelen Feb 1, 2018

Owner

@jackfranklin nice PR 😄

Owner

keppelen commented Feb 1, 2018

@jackfranklin nice PR 😄

@keppelen keppelen merged commit 5f360dc into keppelen:master Feb 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@jackfranklin

This comment has been minimized.

Show comment
Hide comment
@jackfranklin

jackfranklin Feb 1, 2018

Contributor

@keppelen thank you! Would you mind publishing to npm / allowing me to do that? :)

Contributor

jackfranklin commented Feb 1, 2018

@keppelen thank you! Would you mind publishing to npm / allowing me to do that? :)

@jackfranklin

This comment has been minimized.

Show comment
Hide comment
@jackfranklin

jackfranklin Feb 1, 2018

Contributor

Also just realised I should update the README with this, I'll do that tomorrorw.

Contributor

jackfranklin commented Feb 1, 2018

Also just realised I should update the README with this, I'll do that tomorrorw.

@keppelen

This comment has been minimized.

Show comment
Hide comment
Owner

keppelen commented Feb 1, 2018

@jackfranklin

This comment has been minimized.

Show comment
Hide comment
@jackfranklin

jackfranklin Feb 1, 2018

Contributor

🎉 thank you!

Contributor

jackfranklin commented Feb 1, 2018

🎉 thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment