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

[feature]: Order confirmation Page #2288

Merged
merged 37 commits into from
Apr 14, 2020

Conversation

sirugh
Copy link
Contributor

@sirugh sirugh commented Mar 26, 2020

Description

The order confirmation page displays data from the order that was just placed. It's different from most of our other recent components in that it must render "past" data that is no longer stored/cached.

I approached this by prefixing a query on the submission handler that fetches current order details right before placing the order. The details are then passed to the order confirmation component for rendering. The only queries/mutations that exist within the order confirmation page and its children are those around creating an account.

Note:

We combined the subscribe and create account forms into a single form due to restrictions from graphql.

Refer a friend is also deferred.

Related Issue

Closes PWA-186.

Acceptance

Verification Stakeholders

Specification

Verification Steps

  1. As a guest, place an order. See that the order confirmation page appears.
  2. Create an account using the form. It should log you in but not redirect you for now. Eventually this will redirect to an account view or orders page.
  3. As an authed user, place an order. See that the order confirmation page appears but there is no create account form.

If shipping info/method/payment info sections are not merged you may need to manually submit this information to the cart through graphql queries. I'll provide some that should work, just replace cart id with your own from the store.

To add shipping address/method

Go to the /cart page and estimate your shipping. That will attach a fake address as well as select a shipping method for your cart.

To add payment method:

mutation {
    setPaymentMethodOnCart(input: {
        cart_id: "e3ih8dF98xpiykcHkHqb9XSsppOSymMK"
        payment_method: {
            code: "checkmo"
        }
    }) {
        cart {
        selected_payment_method {
            code
        }
        }
    }
}

To add billing address:

mutation {
  setBillingAddressOnCart(
    input: {
      cart_id: "e3ih8dF98xpiykcHkHqb9XSsppOSymMK"
      billing_address: {
        address: {
          firstname: "John"
          lastname: "Doe"
          company: "Company Name"
          street: ["320 N Crescent Dr", "Beverly Hills"]
          city: "Los Angeles"
          region: "CA"
          postcode: "90210"
          country_code: "US"
          telephone: "123-456-0000"
          save_in_address_book: false
        }
      }
    }
  ) {
    cart {
      billing_address {
        firstname
        lastname
        company
        street
        city
        region{
          code
          label
        }
        postcode
        telephone
        country {
          code
          label
        }
      }
    }
  }
}

Screenshots / Screen Captures (if appropriate)

Image from Gyazo

Image from Gyazo

Checklist

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

@m2-community-project m2-community-project bot added this to Ready for Review in Pull Request Progress Mar 26, 2020
@PWAStudioBot
Copy link
Contributor

PWAStudioBot commented Mar 26, 2020

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-186.

Generated by 🚫 dangerJS against 4553675

@@ -19,7 +23,8 @@ const ItemsReview = props => {
const talonProps = useItemsReview({
queries: {
getItemsInCart: LIST_OF_PRODUCTS_IN_CART_QUERY
}
},
data: props.data
Copy link
Contributor Author

@sirugh sirugh Mar 31, 2020

Choose a reason for hiding this comment

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

I want to re-use this component's UI and other logic but not the queries. I'm open to other approaches but this one seemed the most direct. If data is passed to the talon it will use it instead of making a query. Everything else remains the same.

@m2-community-project m2-community-project bot moved this from Ready for Review to Review in Progress in Pull Request Progress Mar 31, 2020
* and then, on success, invokes the `onSubmit` prop, which is usually the action.
*
* This talon is almost identical to the other useCreateAccount but does not
* return `isSignedIn`.
Copy link
Contributor Author

@sirugh sirugh Apr 1, 2020

Choose a reason for hiding this comment

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

There was a great discussion on our internal slack today around this. For now I will leave the implementations of the useCreateAccount talons in place but at some point we will want to come back and extract the logic into a utility hook.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree. Good to have it documented here in a comment.

@sirugh sirugh marked this pull request as ready for review April 1, 2020 15:00
sirugh and others added 5 commits April 1, 2020 16:17
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
Signed-off-by: sirugh <rugh@adobe.com>
supernova-at
supernova-at previously approved these changes Apr 13, 2020
@supernova-at
Copy link
Contributor

Looks good to me. Probably want @jimbo 's sign off on a new dependency though.

@@ -1,34 +1,122 @@
import React from 'react';
import React, { useEffect } from 'react';
import { propType } from 'graphql-anywhere';
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe rename this on import? Open to suggestions here, but I find this pretty vague.

Suggested change
import { propType } from 'graphql-anywhere';
import { propType as queryData } from 'graphql-anywhere';

@jimbo
Copy link
Contributor

jimbo commented Apr 13, 2020

As for the new dependency, it's fine as long as it's only being used for its propType export. Would like confirmation that it's not being included in the production bundle though.

@sirugh
Copy link
Contributor Author

sirugh commented Apr 13, 2020

You can see that most of the code is not included. Not sure why the small amount is leftover in prod.

Develop:

Image from Gyazo

Prod:

@sirugh
Copy link
Contributor Author

sirugh commented Apr 13, 2020

Couldn't find the source code for graphql-anywhere and since there is 1.5kb in the prod bundle for some reason we removed the dependency. Goodnight sweet prince.

jimbo
jimbo previously approved these changes Apr 13, 2020
@dpatil-magento
Copy link
Contributor

@sirugh Pls check the conflict and following observation > on clicking Create Account button.

image

Copy link
Contributor

@tjwiebell tjwiebell left a comment

Choose a reason for hiding this comment

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

72e8ccc and e1938db look good 👍

@m2-community-project m2-community-project bot moved this from Review in Progress to Reviewer Approved in Pull Request Progress Apr 14, 2020
@dpatil-magento dpatil-magento merged commit 2a56b83 into develop Apr 14, 2020
@dpatil-magento dpatil-magento deleted the rugh/pwa-186-order-confirmation-page branch April 14, 2020 20:49
@m2-community-project m2-community-project bot moved this from Reviewer Approved to Done in Pull Request Progress Apr 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg:peregrine pkg:venia-ui version: Minor This changeset includes functionality added in a backwards compatible manner.
Development

Successfully merging this pull request may close these issues.

None yet

7 participants