-
Notifications
You must be signed in to change notification settings - Fork 514
Refactor the standard integration guide #61
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
Conversation
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 amazing!
<script src="https://www.paypal.com/sdk/js?client-id=test¤cy=USD"></script> | ||
<script> |
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.
one thing i would love to see is all of our examples using 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.
I'd be open to that. How do you think they should use paypal-js
in a repo like this? Should we recommend a separate client-side js file that imports in paypal-js and then serve up a bundle via rollup or webpack? Or host it from paypalobjects.com and have folks download it from there?
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 later. I think ideally we'd have something that was as easy as stripe's: https://stripe.com/docs/libraries/stripejs-esmodule#web-stripejs-html
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 with the former, we have to start talking about build tools and that always sucks
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 hear ya on the burden of teaching build tools. I like offering both the CDN option and the ES Module like Stripe does. I'll take this paypal-js loader suggestion back to the team to get more 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.
Looks great! Left a few non-blocking comments
// parse post params sent in body in json format | ||
app.use(express.json()); | ||
|
||
app.post("/my-server/create-paypal-order", async (req, res) => { |
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 tripped me up the other day thinking my-server
was not an actual route...
Thanks for the feedback everyone! I applied some of the recommendations and I'm going to merge this PR. @cnallam, fyi I changed two things with the codespaces integration for standard checkout:
I've successfully tested the CodeSpaces integration off this feature branch. Here are some screenshots for reference: ![]() ![]() ![]() |
The DevRel team has been working on improving the Standard Integration code snippets across developer.paypal.com. This PR includes many of these new changes.
Improvements include:
PayPal-Mock-Response
request header for order creation and capture.alert()
since it pauses the popup flow. Replacing this with appending UI messages to the button container.There are some other changes I am less confident on and could use feedback on.
Feedback welcome! Let us know what you like and don't like.