[PWA-239] Shipping Method (Cart) #2123

tjwiebell · 2020-01-28T22:50:01Z

Description

When a user (shopper) is in the cart flow for Venia, a shipping method selection/estimate component should display in order to allow a shopper to determine the impact of shipping to their order summary.

Requirements:

Adhere to design specs/flows
Initial design accounts for unauthenticated cart/checkout, authenticated to be a fast follow
out of scope:

detecting location based on customer device

Related Issue

[PWA-239] Shipping Method (Cart)

Acceptance

Verification Stakeholders

Specification

Verification Steps

If you want to verify that third-party shipping providers are working, you'll need inventory items that have weight, and your shipping methods configured in the backend. I've done this with a staging environment, you'll have to use Carmina Necklace: https://mcstaging-jnz3dtiuj77ca.dummycachetest.com/

Add product to your cart
Navigate to /cart page and verify that estimate shipping fields are displayed (country, state, zip)
Enter values for these fields and shipping methods should be displayed based on available methods
Select a method and verify that PriceSummary is updated to reflect this selection

Screenshots / Screen Captures (if appropriate)

Checklist

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

…ping_methods

- Update select input to support id as element key - Add disabled style to select

- Dependently display shipping methods container

…ping_methods

- Handle styling of radio option

PWAStudioBot · 2020-01-28T22:54:59Z

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

Generated by 🚫 dangerJS against c5cfe68

…some prop type validation

…ping_methods

sirugh

Nice work! Nothing major stands out to me other than the ungodly amount of time it can take to perform mutations once you've set a method...

sirugh · 2020-01-29T19:06:54Z

packages/venia-ui/lib/components/Select/select.js

-        const options = items.map(({ label, value }) => (
-            <Option key={value} value={value}>
+        const options = items.map(({ id = null, label, value }) => (
+            <Option key={id || value} value={value}>


Add id to items shape. Also why?

If this is because of a duplicate value in a response maybe its a bug?

Discussed IRL, created https://jira.corp.magento.com/browse/MC-30886

sirugh · 2020-01-29T19:49:58Z

packages/venia-ui/lib/components/RadioGroup/radioGroup.js

+                // TODO: these classes may not be defined!
+                classes={{
+                    label: classes.radioLabel,
+                    root: classes.radio


I looked and couldn't find any other reference to the RadioGroup component outside of your usage in this PR. Perhaps we can just update the styles for it instead of overriding classes.

These styles were a little opinionated about the label, using grid to flow content based on screen size, which I wouldn't want to make the default style. Decided to leave this as-is.

sirugh · 2020-01-29T19:53:21Z

packages/venia-ui/lib/components/RadioGroup/radioGroup.js

@@ -40,7 +51,9 @@ export class RadioGroup extends Component {
                        {options}
                    </BasicRadioGroup>
                </div>
-                <Message fieldState={fieldState}>{message}</Message>
+                <Message className={classes.message} fieldState={fieldState}>


Interesting -- was this just not using the class?

Inherited some of the UI work from @supernova-at, who made this change, but from what I can tell none of the styles were extensible, which is what was fixed here.

sirugh · 2020-01-29T20:11:28Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/priceAdjustments.js

-                    <a href="https://jira.corp.magento.com/browse/PWA-239">
-                        Shipping Methods to be completed by PWA-239.
-                    </a>
+                    <ShippingMethods />


Can you set this to isOpen? I accidentally left the coupon code as the default one (and that got merged).

Done in 9c94499. UX would actually prefer this is closed on first render, but given the bug that closes all panels every time an effect is fired, I'm going to suggest leaving open for now.

sirugh · 2020-01-29T20:24:27Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/ShippingMethods/shippingRadios.js

+        ) {
+            cart {
+                id
+                ...PriceSummaryFragment


I prefer to refetch the entire cart page fragment (which includes price summary) because we (the shipping mutation) don't know what else may need eventually want to re-render based on the mutation. For example maybe we have a new UI component that needs to show the chosen option.

Fixed in 9c94499

sirugh · 2020-01-29T20:56:48Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/ShippingMethods/shippingMethods.js

+        <Form className={classes.root}>
+            <ShippingFields
+                selectedShippingFields={selectedShippingFields}
+                setIsFetchingMethods={setIsFetchingMethods}


I don't think you need this state -- you can define the mutations in this component (or talon) and expose the loading state from there. Then you don't need to pass in a callback here and you won't need the effect inside the shipping fields talon, you'd just pass the handleSubmit callback which would invoke the mutation.

I'll look into this. I was constrained by the mutations access to the form fields, but maybe there is another way to gain access outside of the form with a submit handler.

Re-factored this state sync prop out in 9c94499.

sirugh · 2020-01-29T20:58:11Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/ShippingMethods/shippingFields.js

+                <TextInput
+                    field="zip"
+                    initialValue={zip}
+                    onBlur={handleZipOnBlur}


If this field is required for the methods to be fetched we may want to indicate that somehow.

Re-factored to use forms submit handlers, should be more clear what triggers the fetch of methods now.

sirugh · 2020-01-29T20:59:04Z

packages/peregrine/lib/talons/CartPage/PriceAdjustments/ShippingMethods/useShippingRadios.js

+
+    const handleShippingSelection = useCallback(
+        value => {
+            const [carrierCode, methodCode] = value.split('|');


Clever, but can value not be an object?

Man, you missed a thrilling discussion in Slack about this. tl;dr - no, we should strive for rendered markup to still be valid, and radio values must be of type DomString. Would love your opinion on this as well.

https://magento.slack.com/archives/G8N21AQTV/p1580161395028400

sirugh · 2020-01-29T21:05:25Z

packages/peregrine/lib/talons/CartPage/PriceAdjustments/ShippingMethods/useShippingFields.js

+                label: region.name,
+                value: region.code
+            }));
+            formattedStatesData.unshift({ label: '', value: '' });


You do this to present an empty state initially, right? Rather than allowing it as an option maybe we should set the initial selection to <option disabled selected value> -- Select a FOO -- </option> inside the Select component as a default (or configurable) UI.

Oh, good catch. I intended to add support for disabled and hidden props to the Select component options and just completely forgot, will get those added.

Added these props in 9c94499

sirugh · 2020-01-29T21:08:35Z

packages/peregrine/lib/talons/CartPage/PriceAdjustments/ShippingMethods/useShippingFields.js

+    }
+
+    let formattedStatesData = [];
+    if (fetchStatesCalled && !isStatesLoading && !statesError) {


We need to have a consensus on loading states at some point. Did UX direct you to intentionally show "loading countries" but not "loading states/regions"?

No direction, was waiting on demo to confirm those details. There's a bigger problem here too with non-US countries that don't even have state options, and how to handle that. Will get that lined up for tomorrow.

- Adjust some minor things based on UX feedback

supernova-at

Graceful handling of shipping methods not loading is really the only thing (see #2123 (comment)).

Great job!

- Update test snaps after mainline merge

- Update tests

- Memoize click event for enabling form

...artPage/PriceAdjustments/ShippingMethods/__tests__/__snapshots__/shippingRadios.spec.js.snap

supernova-at · 2020-02-06T15:13:51Z

packages/venia-ui/lib/components/CartPage/PriceAdjustments/ShippingMethods/shippingForm.js

@@ -82,7 +82,7 @@ const ShippingForm = props => {
                {!hasMethods ? (
                    <Button
                        classes={{ root_normalPriority: classes.submit }}
-                        disabled={setShippingLoading ? true : null}
+                        disabled={setShippingLoading}


Took me a second to parse this one because setShippingLoading reads like a function, not a boolean.

Up to you if you want to change it or not.

Yep, renamed to follow our bool naming convention in cf8fd2e, good call.

Want to memoize the showForm function.

schensley

UX approved.
Nice work!

…ping_methods

dpatil-magento · 2020-02-11T18:34:02Z

@tjwiebell Other than shipping option ordering issue rest all looks good.

sirugh · 2020-02-11T20:35:07Z

There is an issue with GQL response returning incorrect values for flatrate, but that should be handled separately. If @dpatil-magento made a ticket can you post it here? Thanks!

dpatil-magento · 2020-02-11T21:35:16Z

@sirugh Its https://jira.corp.magento.com/browse/MC-31420

supernova-at and others added 10 commits January 13, 2020 13:53

Rough UI

aa7b313

ZIP input moves in mobile view

4bd3fd8

Shipping-specific Radio groups styling

1223035

Separate address and methods collection

1a1a4ce

Merge remote-tracking branch 'origin/develop' into supernova/239_ship…

d72165f

…ping_methods

- Nest component under parent- Fixup some styling based on mocks

cb571dc

- Start hooking up data fetching with different steps of form

781a124

- Update select input to support id as element key - Add disabled style to select

- Finish modifications to shipping radio options

272799e

- Dependently display shipping methods container

Merge remote-tracking branch 'origin/develop' into supernova/239_ship…

bf76f80

…ping_methods

- Fetch initial form state from graphql and wait to render

9a65c2e

- Handle styling of radio option

tjwiebell added the version: Minor This changeset includes functionality added in a backwards compatible manner. label Jan 28, 2020

m2-community-project bot added this to Ready for Review in Pull Request Progress Jan 28, 2020

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Jan 28, 2020

tjwiebell added 3 commits January 29, 2020 12:06

- Manage loading state of siblings and mask input while loading- Add …

230cbeb

…some prop type validation

Merge remote-tracking branch 'origin/develop' into supernova/239_ship…

c693358

…ping_methods

- Remove export thats no longer present after merge

030c9dc

tjwiebell marked this pull request as ready for review January 29, 2020 18:28

sirugh requested changes Jan 29, 2020

View reviewed changes

m2-community-project bot assigned sirugh Jan 29, 2020

m2-community-project bot moved this from Ready for Review to Changes Requested in Pull Request Progress Jan 29, 2020

m2-community-project bot added the Progress: changes requested label Jan 29, 2020

m2-community-project bot assigned tjwiebell Jan 29, 2020

m2-community-project bot moved this from Changes Requested to Review in Progress in Pull Request Progress Jan 29, 2020

m2-community-project bot added Progress: review and removed Progress: changes requested labels Jan 29, 2020

tjwiebell added 3 commits January 29, 2020 15:32

- Add shippingMethods tests

634c3e0

- Adjust some minor things based on UX feedback

- Refactor based on UX feedback and code review

9c94499

- Remove unused prop

f90c38a

supernova-at requested changes Feb 5, 2020

View reviewed changes

tjwiebell added 4 commits February 5, 2020 14:08

Merge branch 'develop' into supernova/239_shipping_methods

b033585

- Address PR feedback

23e9cb5

- Update test snaps after mainline merge

- Use value instead of casting false to null

f9a3c8e

- Update tests

- Fix GraphQL validation errors

2746e6c

- Memoize click event for enabling form

supernova-at reviewed Feb 6, 2020

View reviewed changes

...artPage/PriceAdjustments/ShippingMethods/__tests__/__snapshots__/shippingRadios.spec.js.snap Show resolved Hide resolved

supernova-at reviewed Feb 6, 2020

View reviewed changes

- Rename variable for clarity

cf8fd2e

supernova-at previously approved these changes Feb 6, 2020

View reviewed changes

schensley previously approved these changes Feb 7, 2020

View reviewed changes

m2-community-project bot assigned schensley Feb 7, 2020

tjwiebell added 2 commits February 11, 2020 08:37

Merge remote-tracking branch 'origin/develop' into supernova/239_ship…

68a857c

…ping_methods

- Fixup unit test after merge

5596146

tjwiebell dismissed stale reviews from schensley and supernova-at via 5596146 February 11, 2020 14:53

dpatil-magento and others added 2 commits February 11, 2020 09:34

Merge branch 'develop' into supernova/239_shipping_methods

f9d67dc

- Fixup tests after mainline merge

b4f6cdb

tjwiebell added 2 commits February 11, 2020 12:54

- Add sort to shipping methods to solve GraphQL response order bug

9e2d855

- Add amount fields to mocked data to fix test

c5cfe68

m2-community-project bot assigned jimbo Feb 11, 2020

sirugh approved these changes Feb 11, 2020

View reviewed changes

dpatil-magento merged commit 9346ba2 into develop Feb 11, 2020

dpatil-magento deleted the supernova/239_shipping_methods branch February 11, 2020 21:35

m2-community-project bot moved this from Review in Progress to Done in Pull Request Progress Feb 11, 2020

m2-community-project bot removed the Progress: review label Feb 11, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PWA-239] Shipping Method (Cart) #2123

[PWA-239] Shipping Method (Cart) #2123

tjwiebell commented Jan 28, 2020

PWAStudioBot commented Jan 28, 2020 •

edited

sirugh left a comment

sirugh Jan 29, 2020

sirugh Jan 29, 2020

tjwiebell Jan 29, 2020

sirugh Jan 29, 2020

tjwiebell Feb 5, 2020

sirugh Jan 29, 2020

tjwiebell Jan 29, 2020

sirugh Jan 29, 2020

tjwiebell Feb 5, 2020

sirugh Jan 29, 2020

tjwiebell Feb 5, 2020

sirugh Jan 29, 2020

tjwiebell Jan 29, 2020

tjwiebell Feb 5, 2020

sirugh Jan 29, 2020

tjwiebell Feb 5, 2020

sirugh Jan 29, 2020

tjwiebell Jan 29, 2020 •

edited

sirugh Jan 29, 2020

tjwiebell Jan 29, 2020

tjwiebell Feb 5, 2020

sirugh Jan 29, 2020

tjwiebell Jan 29, 2020

supernova-at left a comment

supernova-at Feb 6, 2020

tjwiebell Feb 6, 2020

schensley left a comment

dpatil-magento commented Feb 11, 2020

sirugh commented Feb 11, 2020

dpatil-magento commented Feb 11, 2020

[PWA-239] Shipping Method (Cart) #2123

[PWA-239] Shipping Method (Cart) #2123

Conversation

tjwiebell commented Jan 28, 2020

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jan 28, 2020 • edited

sirugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tjwiebell Jan 29, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

supernova-at left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schensley left a comment

Choose a reason for hiding this comment

dpatil-magento commented Feb 11, 2020

sirugh commented Feb 11, 2020

dpatil-magento commented Feb 11, 2020

PWAStudioBot commented Jan 28, 2020 •

edited

tjwiebell Jan 29, 2020 •

edited