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

chore(docs): Adds two key steps on building ecommerce sites with Stripe tutorial #18669

Merged
merged 22 commits into from
Nov 14, 2019

Conversation

urielhdz
Copy link
Contributor

@urielhdz urielhdz commented Oct 14, 2019

Description

After following the tutorial on how tu build ecommerce sites with Stripe I noticed that two important steps were missing from the tutorial:

  1. SKUs need to have a name attribute for them to be used on the client only Checkout integration: I added the steps to add the name through the Stripes dashboard, although the attribute can also be set through the API, I think we could fix this issue by just adding the requirement as an annotation instead of explaining the whole process, but since adding the attribute is not that straightforward I added the steps to the tutorial.
  2. Checkout client-side only integration needs to be enabled to use the redirectToCheckout function: This is key since the guide is based on this specific functionality, since this issue is solved only through the Stripes dashboard, I think that is safe to keep the section on how to enable the integration.

Uriel Hernández and others added 12 commits September 21, 2019 00:40
Adds a reference guide on how to upgrade Gatsby and its dependencies
…r the doc-links.yaml file, escaped < and > characters in markdown thata were causing some errors
Co-Authored-By: Marcy Sutton <marcy@gatsbyjs.com>
…emoving yarn instructions and alignments to Gatsby style guide
Co-Authored-By: Marcy Sutton <marcy@gatsbyjs.com>
…ites with Stripe that were required by stripe for the example to work
@urielhdz urielhdz requested a review from a team as a code owner October 14, 2019 22:10
@urielhdz
Copy link
Contributor Author

Seems like the Stripe's interface changed for me 🙃, now SKUs require a name once they are created. I don't know how to tackle these scenarios in which the UI seems to differ per account.

@urielhdz
Copy link
Contributor Author

I removed the section on how to set a name for SKUs since the latest UI requires a name by default.

Now this PR limits to the second point, which reminds you that the Checkout client-side only integration needs to be enabled.

@LekoArts LekoArts added the type: documentation An issue or pull request for improving or updating Gatsby's documentation label Oct 15, 2019
Copy link
Contributor

@LekoArts LekoArts left a comment

Choose a reason for hiding this comment

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

Small comment from me. Also please remove the (now) two unrelated images from this PR. Thanks!

docs/tutorial/ecommerce-tutorial/index.md Outdated Show resolved Hide resolved
@urielhdz
Copy link
Contributor Author

Upon further investigation, It seems like the SKUs interface changes after the "Checkout client-only integration" is enabled.

@urielhdz urielhdz changed the title chore(docs): Adds two key steps on building ecommer sites with Stripe tutorial chore(docs): Adds two key steps on building ecommerce sites with Stripe tutorial Oct 16, 2019
urielhdz and others added 3 commits October 16, 2019 10:41
@urielhdz
Copy link
Contributor Author

Thanks to both @muescha and @LekoArts for your improvements.

Given how the SKUs interface changes depending on your Stripe's settings, and the fact that you may be using the API and not the dashboard, I rewrote the Create a product and SKU to mention that your SKUs need to have a name for them to be used on this integration, and because there was a reference of Stripe working with any Backend component.

I also created a new account to follow the process from the start and I added instruction on how to properly setup your account to use this integration, this includes enabling the 'Checkout client-only integration' and setting a name for your account.

I also changed some pronouns from 'we' to 'you' that were left on the tutorial to adhere to #18284.

Additionally, I was wondering if components should be changed from class based to functional? In this particular example we would need to change both the tutorial and the provided code from the demo.

Thanks again!

@muescha
Copy link
Contributor

muescha commented Oct 16, 2019

i think updating it to functional/hooks would be a nice improvement.

@urielhdz
Copy link
Contributor Author

i think updating it to functional/hooks would be a nice improvement.

I already have something working, let me push it to the PR

@muescha
Copy link
Contributor

muescha commented Oct 16, 2019

wow great!

@urielhdz
Copy link
Contributor Author

wow great!

I am going to fork the example to test the code and the changes, there are multiple components that need to be changed, I don't know if it is best to open a new PR to make changes easier to review or do you agree on pushing them here? @muescha

@muescha
Copy link
Contributor

muescha commented Oct 16, 2019

you are right, i also think it is better to make an additional request (after this is merged) to make review easier.

@urielhdz
Copy link
Contributor Author

Let me know if there is anything missing for this to be merged 😅

@muescha muescha requested review from marcysutton and a team October 16, 2019 23:35
@urielhdz urielhdz dismissed LekoArts’s stale review October 18, 2019 14:53

The changes were introduced in a separate commit

@LekoArts
Copy link
Contributor

Let's wait for @gatsbyjs/learning to comment on that, too!

Copy link
Contributor

@laurieontech laurieontech left a comment

Choose a reason for hiding this comment

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

Made a couple minor text changes, but overall this is looking really good. Should be ready for a final review.

Co-Authored-By: LB <laurie@gatsbyjs.com>
@urielhdz
Copy link
Contributor Author

Thank you @laurieontech ! I added your suggestions to the PR!

Copy link
Contributor

@marcysutton marcysutton left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! I left some suggestions of things we could improve, but they're pretty minor. Let us know if you have any questions!

docs/tutorial/ecommerce-tutorial/index.md Outdated Show resolved Hide resolved
docs/tutorial/ecommerce-tutorial/index.md Outdated Show resolved Hide resolved
docs/tutorial/ecommerce-tutorial/index.md Outdated Show resolved Hide resolved
Co-Authored-By: Marcy Sutton <marcy@gatsbyjs.com>
@urielhdz
Copy link
Contributor Author

Thanks, @marcysutton! I just applied your suggestions, let me know if is there anything else I can do for this to be merged

@urielhdz
Copy link
Contributor Author

urielhdz commented Nov 6, 2019

Hello! Let me know if is there anything that needs to be done for this to be merged and I'd be happy to work on it!

@marcysutton @laurieontech

Thanks!

@gillkyle gillkyle self-assigned this Nov 14, 2019
@gillkyle
Copy link
Contributor

gillkyle commented Nov 14, 2019

Hey @urielhdz, I know this PR has been up and you want to get it over the finish line (and thanks so much for adding your improvements!), I'm going to do my best to look over this today or tomorrow so we can merge it in. Just want to let you know so you're not in the dark 🙂

Copy link
Contributor

@gillkyle gillkyle left a comment

Choose a reason for hiding this comment

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

Just reviewed and looks good to me! This is a feature I wasn't aware of, when this tutorial was written a couple years ago it used a serverless lambda function to process the payment. Very neat and thanks for patching up this inconsistency 🙂

@gillkyle gillkyle added the bot: merge on green Gatsbot will merge these PRs automatically when all tests passes label Nov 14, 2019
@gillkyle gillkyle merged commit 58a627e into gatsbyjs:master Nov 14, 2019
rickiesmooth pushed a commit to rickiesmooth/gatsby that referenced this pull request Nov 17, 2019
…byjs#18669)

* resolves gatsbyjs#14598

Adds a reference guide on how to upgrade Gatsby and its dependencies

* Fixed typos and added related content

* Fixed additional typos

* Fixed additional typos, one with the name of the file, added links for the doc-links.yaml file, escaped < and > characters in markdown thata were causing some errors

* Updated title based on @marcysutton suggestions

Co-Authored-By: Marcy Sutton <marcy@gatsbyjs.com>

* Solves suggested changes such as modifying links for relative urls, removing yarn instructions and alignments to Gatsby style guide

* Apply additional suggestions

Co-Authored-By: Marcy Sutton <marcy@gatsbyjs.com>

* Little typo with the npm-force-resolutions package name is fixed

* Added two additional steps for the tutorial on how to build ecommer sites with Stripe that were required by stripe for the example to work

* Added missing screenshot files

* Removed intstructions from the old Stripe UI

* Removes the now unused images and integrates suggestions from gatsby learning

* Changed integration to function when referring to the `redirectToCheckout` function

Co-Authored-By: Michael <184316+muescha@users.noreply.github.com>

* Rewrote the requirements to use this integration, changed pronouns, fixed typos, changed quotes

* Added missing screenshot file

* Apply suggestions from code review

Co-Authored-By: LB <laurie@gatsbyjs.com>

* Apply suggestions from code review

Co-Authored-By: Marcy Sutton <marcy@gatsbyjs.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot: merge on green Gatsbot will merge these PRs automatically when all tests passes type: documentation An issue or pull request for improving or updating Gatsby's documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants