bugfix: do not render things when no items are in cart #2112

sirugh · 2020-01-22T20:26:38Z

Description

We were rendering the PriceAdjustments and PriceSummary components regardless of whether we should.

This PR adds a getCartDetails query that does an initial fetch for cart information, including total_quantity which it then uses to conditionally render the subcomponents.

This PR also removes the refetchQueries from the removeItem mutation. It now uses named fragments! This means that after the remove item mutation is called you should see no subsequent refetches for new data, and yet the UI should update!

Related Issue

Closes PWA-326.

Acceptance

Verification Stakeholders

@schensley @jimbo

Specification

Verification Steps

Clear your storage and go to /cart. You should see no items, no adjustments, and no summary.
Add a product and go back to `/cart. You should see the item, adjustments, and summary.
Remove the item with the kebab toggle. The item, adjustments, and summary views should disappear.

Screenshots / Screen Captures (if appropriate)

Checklist

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

… or price summary when no items are in cart Signed-off-by: Stephen Rugh <rugh@adobe.com>

PWAStudioBot · 2020-01-22T20:32:58Z

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

Generated by 🚫 dangerJS against f21e8e4

schensley · 2020-01-22T20:37:32Z

@sirugh while this functions as it is supposed to, I think some addition finesse could be given to the 'fading out' of elements.
Can this behave in such a way that first the removed product line fades out, and then the Pricing Summary and the accordion of modules (Shipping, Coupon, etc.) fades out after? It feels abrupt to have all fade out (at least so quickly). Thoughts?

sirugh · 2020-01-22T20:41:01Z

packages/venia-ui/lib/components/CartPage/cartPageFragments.js

+
+export const CartPageFragment = gql`
+    fragment CartPageFragment on Cart {
+        total_quantity


Should this fragment just compose itself of the other block components ie:

export const CartPageFragment = gql` fragment CartPageFragment on Cart { total_quantity ...PriceSummaryFragment ...PriceAdjustmentsFragment ...ProductListingFragment } `

And then in mutations, instead of requesting individual fragments we could just request the CartPageFragment?

export const REMOVE_ITEM_MUTATION = gql` mutation removeItem($cartId: String!, $itemId: Int!) { removeItemFromCart(input: { cart_id: $cartId, cart_item_id: $itemId }) { cart { id ...CartPageFragment ...PriceSummaryFragment ...PriceAdjustmentsFragment ...ProductListingFragment } } } `;

vs

export const REMOVE_ITEM_MUTATION = gql` mutation removeItem($cartId: String!, $itemId: Int!) { removeItemFromCart(input: { cart_id: $cartId, cart_item_id: $itemId }) { cart { id ...CartPageFragment } } } `;

The former approach is more "slim" in that it requests specific data whereas the latter just does a bulk fetch on mutate.

I like the idea of an encapsulating page fragment, which future proofs new features. When new data is needed on the page, a new fragment is added to the page fragment, and any existing mutations automatically pick it up. Easier to maintain, but also relies on the assumption that over fetching doesn't cause unnecessary renders.

Seems like over fetching would only cause re-renders if the data changes.

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh · 2020-01-22T21:12:08Z

packages/venia-ui/lib/components/CartPage/ProductListing/product.js

@@ -95,7 +92,9 @@ export const REMOVE_ITEM_MUTATION = gql`
        removeItemFromCart(input: { cart_id: $cartId, cart_item_id: $itemId }) {
            cart {
                id
+                ...CartPageFragment


When performing a mutation within the CartPage we can just refetch the entire composed fragment rather than requiring each component to fetch specifics. This may result in "overfetch" such as in the case of adding a coupon where you only really need to update the total and discounts.

I'm fine with this, under the assumption that over fetching doesn't causes unnecessary re-renders if that data didn't change.

I'll have to look -- I'm not sure, it seems super fast.

For now I left the fetch policy so this doesn't matter.

sirugh · 2020-01-22T21:13:38Z

packages/venia-ui/lib/components/CartPage/ProductListing/productListingFragments.js

+        id
+        items {
+            id
+            product {


The props requested in items should probably be moved to a productListingProductFragment object that is then used here. The idea is that each visual component declares what it needs to render, and the productListing component is really just a wrapper that passes the data down to the product component anyways.

I like this idea, a great extension of what you did with PriceSummary. This did bring to my attention we're inconsistently colocating fragments though, and should probably decide on a single location.

Dedicated file (<componentName>Fragments.js)

Named export in component (eg. discountSummary)

Of course the Apollo docs use static properties, which we know has the issue of cyclical dependencies when children need parent fragments (also an issue with named export). Maybe this is another reason to switch to a single root component query, and use apollo-anywhere filter. This still would require CartPageFragment be exported outside of the root component though...

Tough questions, but I'm okay iterating on it and not solving in the scope of this PR.

Yea I just wanted to call it out :)

sirugh · 2020-01-22T21:14:54Z

packages/venia-ui/lib/components/CartPage/cartPage.js

@@ -37,11 +42,11 @@ const CartPage = props => {
                    <ProductListing />
                </div>
                <div className={classes.price_adjustments_container}>
-                    <PriceAdjustments />
+                    {hasItems ? <PriceAdjustments /> : null}


Should we do the same here for ProductListing? That component (and PriceSummary, actually) have a null render state based on the data they receive back.

If we add a conditional before rendering though, we prevent the component from mounting/making a graphQL query, which could be a win. Thoughts?

sirugh · 2020-01-22T21:15:47Z

packages/venia-ui/lib/queries/getCartDetails.graphql

@@ -1,5 +1,6 @@
 query getCartDetails($cartId: String!) {
    cart(cart_id: $cartId) {
+        id


This was missing, and potentially might cause cache misses.

However, in order for the trigger to be updated after mutations like removeItem we would have to start using named fragments in this query. For now we can just refetch.

These cart queries use different versions of schema, so with the inclusion of id heuristic fragment matching wouldn't hit (must be exact). Even if we switched to consuming fragments here, we'd have to update the components to the new schema, which is making the scope creepy.

If this was a priority, I would probably just do a manual cache update in our mutations.

As we discovered just now this works because Apollo sees "partial data" and thus makes a network call.

sirugh · 2020-01-22T21:20:48Z

I think some addition finesse could be given to the 'fading out' of elements. Can this behave in such a way that first the removed product line fades out, and then the Pricing Summary and the accordion of modules (Shipping, Coupon, etc.) fades out after? It feels abrupt to have all fade out (at least so quickly). Thoughts?

@schensley I definitely think it can be touched up, just not in the scope of this ticket. We should make a follow-up. Can you start that conversation with Andrew as it'll be good to get some mocks that show transition requirements as in what order things disappear and the exact timing of each?

tjwiebell

Minor tweaks, and replies to your callouts. Looks good, should be a huge improvement.

tjwiebell · 2020-01-22T21:23:12Z

packages/venia-ui/lib/components/CartPage/ProductListing/product.js

@@ -95,7 +92,9 @@ export const REMOVE_ITEM_MUTATION = gql`
        removeItemFromCart(input: { cart_id: $cartId, cart_item_id: $itemId }) {
            cart {
                id


Don't need this id stub anymore since you're using the fragment.

So fragments should always include it? This was one of those things I wasn't sure about. Do we just always make sure to have some identifier for caching in our queries/mutations or can we assume that fragment authors will include it?

Yes, I think we should make this assumption that all fragments include id when available. Ad-hoc inclusion of id would imply we're relying on heuristic fragment matching, which requires exact field matches, which is difficult to maintain. It's not causing any harm, but could lead to confusion that you always need to add an id field, when you don't.

Do you still feel this way? I kind of prefer that we let fragments only define the data they need.

I don't think fragments should need any additional config besides inclusion to work; without id in the fragment, developers would need to be aware they need it for cache updates to work.

It makes sense to me that the base "wrapping" query would include the id field, not the fragments.

This seems like it could go either way -- either way we need to include id somewhere and it does not hurt to declare it multiple times so I'll just leave it as is.

Great compromise 🤝

tjwiebell · 2020-01-22T21:25:54Z

packages/venia-ui/lib/components/CartPage/ProductListing/product.js

@@ -95,7 +92,9 @@ export const REMOVE_ITEM_MUTATION = gql`
        removeItemFromCart(input: { cart_id: $cartId, cart_item_id: $itemId }) {
            cart {
                id
+                ...CartPageFragment


I'm fine with this, under the assumption that over fetching doesn't causes unnecessary re-renders if that data didn't change.

packages/venia-ui/lib/components/CartPage/ProductListing/productListing.js

tjwiebell · 2020-01-22T21:36:11Z

packages/venia-ui/lib/components/CartPage/cartPage.js

@@ -8,9 +9,13 @@ import PriceSummary from './PriceSummary';
 import ProductListing from './ProductListing';
 import { mergeClasses } from '../../classify';
 import defaultClasses from './cartPage.css';
+import { CartPageFragment } from './cartPageFragments';


I would support this overfetch if it prevented network calls in sub-components OR if you drilled the data down. Via chat you confirmed the first is not true; lets use a slimmer fragment that only grabs the data this component needs (total_quantity).

tjwiebell · 2020-01-22T21:49:58Z

packages/venia-ui/lib/components/CartPage/cartPageFragments.js

+
+export const CartPageFragment = gql`
+    fragment CartPageFragment on Cart {
+        total_quantity


I like the idea of an encapsulating page fragment, which future proofs new features. When new data is needed on the page, a new fragment is added to the page fragment, and any existing mutations automatically pick it up. Easier to maintain, but also relies on the assumption that over fetching doesn't cause unnecessary renders.

…art queries to match cache entries Signed-off-by: Stephen Rugh <rugh@adobe.com>

Signed-off-by: Stephen Rugh <rugh@adobe.com>

…d that's out of scope... Signed-off-by: Stephen Rugh <rugh@adobe.com>

Signed-off-by: Stephen Rugh <rugh@adobe.com>

tjwiebell

Death to the refetch! All hail fragments. Nice work, good groundwork to start iterating on this pattern.

Move fragments out of components for reuse. Do not render adjustments…

d15513b

… or price summary when no items are in cart Signed-off-by: Stephen Rugh <rugh@adobe.com>

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

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

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

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

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

m2-community-project bot added the Progress: review label Jan 22, 2020

sirugh commented Jan 22, 2020

View reviewed changes

sirugh added 2 commits January 22, 2020 14:52

Fix useCartPage tests

9f86e86

Signed-off-by: Stephen Rugh <rugh@adobe.com>

Compose fragment to top level cart page and reuse in mutation

7520752

Signed-off-by: Stephen Rugh <rugh@adobe.com>

sirugh commented Jan 22, 2020

View reviewed changes

tjwiebell suggested changes Jan 22, 2020

View reviewed changes

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

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

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

sirugh added 4 commits January 23, 2020 10:26

Reset fetch policies to default cache-first. Use ids in all graphql c…

47c231e

…art queries to match cache entries Signed-off-by: Stephen Rugh <rugh@adobe.com>

Move fragment out of product listing query

cacc123

Signed-off-by: Stephen Rugh <rugh@adobe.com>

Use id in fragments instead of query

4eab43f

Signed-off-by: Stephen Rugh <rugh@adobe.com>

readd fetch policy since other mutations would have to be modified an…

64250fd

…d that's out of scope... Signed-off-by: Stephen Rugh <rugh@adobe.com>

m2-community-project bot assigned supernova-at Jan 23, 2020

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

m2-community-project bot removed the Progress: changes requested label Jan 23, 2020

m2-community-project bot added the Progress: review label Jan 23, 2020

sirugh added 2 commits January 23, 2020 13:51

recompose cart query using child fragments

4bf1767

Signed-off-by: Stephen Rugh <rugh@adobe.com>

do not render listing if no items. fix tests

f21e8e4

Signed-off-by: Stephen Rugh <rugh@adobe.com>

tjwiebell approved these changes Jan 23, 2020

View reviewed changes

dpatil-magento merged commit 0d72488 into develop Jan 23, 2020

dpatil-magento deleted the rugh/cart-no-adjustments-without-items branch January 23, 2020 22:50

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

m2-community-project bot removed the Progress: review label Jan 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bugfix: do not render things when no items are in cart #2112

bugfix: do not render things when no items are in cart #2112

sirugh commented Jan 22, 2020 •

edited

PWAStudioBot commented Jan 22, 2020 •

edited

schensley commented Jan 22, 2020

sirugh Jan 22, 2020

sirugh Jan 22, 2020 •

edited

tjwiebell Jan 22, 2020

sirugh Jan 23, 2020

sirugh Jan 22, 2020

tjwiebell Jan 22, 2020

sirugh Jan 22, 2020

sirugh Jan 23, 2020

sirugh Jan 22, 2020

tjwiebell Jan 23, 2020

sirugh Jan 23, 2020

sirugh Jan 22, 2020

sirugh Jan 22, 2020 •

edited

tjwiebell Jan 23, 2020

sirugh Jan 23, 2020

sirugh commented Jan 22, 2020

tjwiebell left a comment

tjwiebell Jan 22, 2020

sirugh Jan 22, 2020

tjwiebell Jan 23, 2020

sirugh Jan 23, 2020

tjwiebell Jan 23, 2020

supernova-at Jan 23, 2020

sirugh Jan 23, 2020

tjwiebell Jan 23, 2020

tjwiebell Jan 22, 2020

tjwiebell Jan 22, 2020

tjwiebell Jan 22, 2020

tjwiebell left a comment

bugfix: do not render things when no items are in cart #2112

bugfix: do not render things when no items are in cart #2112

Conversation

sirugh commented Jan 22, 2020 • edited

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jan 22, 2020 • edited

schensley commented Jan 22, 2020

Choose a reason for hiding this comment

sirugh Jan 22, 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

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

sirugh Jan 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh commented Jan 22, 2020

tjwiebell 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

tjwiebell left a comment

Choose a reason for hiding this comment

sirugh commented Jan 22, 2020 •

edited

PWAStudioBot commented Jan 22, 2020 •

edited

sirugh Jan 22, 2020 •

edited

sirugh Jan 22, 2020 •

edited