-
Notifications
You must be signed in to change notification settings - Fork 2
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
Explain why the world needs react-paypal-js #27
Conversation
c9b4d2b
to
0bd9cfc
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.
Nice 🎉 one very tiny non-blocking comment.
README.md
Outdated
|
||
- Enforce async loading the JS SDK up front so when it's time to render the buttons to your buyer, they render immediately. | ||
Abstract away the complexity around loading the JS SDK with the global `<PayPalScriptProvider>` component. | ||
- Support dispatching actions to reload the JS SDK and rerender components when global parameters like `currency` change. |
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.
Nit: "rerender" is showing up as a typo for me. is it re-render? 🤷♀️
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.
@gregjopa When I read client-side
I usually couple that with all client-side
apps and not just single page apps
as is this case. Would it be clearer to make this distinguishment?
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.
Good point. I think it is re-render. I updated 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.
@mnicpt good point. I updated to use single page web apps.
lgtm! I don't know how to put this into words (or if we should mention it or how to mention it), but I feel like asking React developers to include a script in their index.html isn't a typical React pattern. Usually React developers import and use a component where they need it. All of the reasons you outlined around loading are the real reasons why we need this. But, there's also something to be said about the React mindset of thinking in terms of components that creates a block in their minds when asking them to integrate the SDK. I don't know how to put this into words, so feel free to take it or leave it :D No need to act on this feedback. |
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.
Great work, @gregjopa !
|
||
Developers integrating with PayPal are expected to add the JS SDK `<script>` to a website and then render components like the PayPal Buttons after the script loads. This architecture works great for simple websites but can be challenging when building single page apps. | ||
|
||
React developers think in terms of components and not about loading external scripts from an index.html file. It's easy to end up with a React PayPal integration that's sub-optimal and hurts the buyer's user experience. For example, abstracting away all the implementation details of the PayPal Buttons into a single React component is an anti-pattern because it tightly couples script loading with rendering. It's also problematic when you need to render multiple different PayPal components that share the same global script parameters. |
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.
@mstuart, I attempted to add your feedback. I re-worded the problem statement. Please have a look.
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.
oops didn't see this earlier, but lgtm!
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.
captured it much better than i could :D
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.
This is great!
|
||
### The Solution | ||
|
||
Provide a solution to developers that abstracts away complexities around loading the JS SDK. Enforce best practices by default so buyers get the best possible user experience. |
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.
Non-blocking comment: It would be nice to reword this sentence to `react-paypal-js provides a solution to developers to abstract away ..... It enforces best practices..."
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.
@shrutikapoor08 that's a great suggestion. I agree. Would you be up for submitting a PR with this change? It would be awesome to get you added to the contributors list for react-paypal-js 😄
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.
YEAH!!
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.
Looks great. I know this PR is merged, but still leaving a few non-blocking comments :)
|
||
Provide a solution to developers that abstracts away complexities around loading the JS SDK. Enforce best practices by default so buyers get the best possible user experience. | ||
|
||
- Enforce async loading the JS SDK up front so when it's time to render the buttons to your buyer, they render immediately. |
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.
[Non-blocking comment:] These lines could use a heading Features:
The goal with PR is to better explain the problems that react-paypal-js is solving. Here's a screenshot this new section in the readme: