Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Countries from GraphQL #1993

Merged
merged 19 commits into from
Nov 26, 2019
Merged

Countries from GraphQL #1993

merged 19 commits into from
Nov 26, 2019

Conversation

supernova-at
Copy link
Contributor

@supernova-at supernova-at commented Nov 21, 2019

Description

This PR updates Venia to fetch countries data from GraphQL instead of the REST endpoint.

Related Issue

PWA-144.

Acceptance

Verification Stakeholders

@sirugh @jimbo @tjwiebell @revanth0212

Specification

Verification Steps

  1. Add an item to your cart
  2. Begin checkout by clicking the "Checkout" button in MiniCart

State Validation Still Works

  1. Fill out the Shipping Address form but don't fill in a state abbreviation. Verify that you get an inline error message.
  2. Fill out the Shipping Address form with an invalid state abbreviation (ex: "GO"). Verify that you get an inline error message.
  3. Fill out the Shipping Address form with an invalid state abbreviation (too many characters) (ex: "GOOSE"). Verify that you get an inline error message.
  4. Verify that the form submit succeeds with a valid state abbreviation
  5. Repeat the steps above for the Billing Address form.

Submit Order Still Works

  1. Submit an order both using "billing address same as shipping address" and not. Verify that both succeed.

Screenshots / Screen Captures (if appropriate)

Checklist

  • I have updated the documentation accordingly, if necessary.
  • I have added tests to cover my changes, if necessary.

@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Nov 21, 2019

Messages
📖

Access a deployed version of this PR here. Make sure to wait for the "pwa-pull-request-deploy" job to complete.

📖 DangerCI Failures related to missing labels/description/linked issues/etc will persist until the next push or next nightly build run (assuming they are fixed).
📖

Associated JIRA tickets: PWA-144.

Generated by 🚫 dangerJS against d36b0d4

@supernova-at supernova-at added the version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump. label Nov 22, 2019
@supernova-at
Copy link
Contributor Author

This is a Major change because we've removed some Redux actions and removed the countries property from Redux.

Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Good work! Only a few things before this can get approved methinks.

@supernova-at supernova-at requested a review from sirugh November 25, 2019 14:10
Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

Sorry, I just realized we have a potential bug so I want to get that remedied before we move this along.


const { data } = useQuery(countriesQuery);

const { countries } = data || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I read this I realize in the past we would make requests for country data in the beginCheckout async action which was awaited in the initial opening of the submission form. This means that we don't render the form until we already have the data.

Now, we could possibly render with an empty object for countries which may be incorrect. I think this means you should add a loading spinner/state and use that when countries is empty (or null/undefined). In this case it's probably safe to just return data since it'll be undefined until then and we can block render until we get an error or it's finished loading.

Copy link
Contributor Author

@supernova-at supernova-at Nov 25, 2019

Choose a reason for hiding this comment

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

Resolved in 4a253fe.

Loading:
(don't worry about the slow spinner image loading, that's unrelated)

loading_checkout

Error:

error_checkout

Let me know what you think of the implementation for sure.

Copy link
Contributor

@sirugh sirugh left a comment

Choose a reason for hiding this comment

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

I'm happy with this! While we did discuss passing invokers to thunks in some cases this particular case doesn't seem necessary. Keeping the query/data as local state to the checkout form is fine.

Good job!

@tjwiebell tjwiebell merged commit 4a5a3e6 into develop Nov 26, 2019
@tjwiebell tjwiebell deleted the supernova/144_gql_countries branch November 26, 2019 22:19
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
pkg:peregrine pkg:venia-ui version: Major This changeset includes incompatible API changes and its release necessitates a Major version bump.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants