[Cart v2] ProductListing Component #2094

tjwiebell · 2020-01-10T19:44:12Z

Description

Within the Cart Page component, if a product has been added to cart it will display with necessary information and actions for a shopper to view/edit/delete.

Requirements:

Adhere to design specs/flows provided
Empty cart should display appropriate messaging
"Sign in" button opens sign in modal that returns back to cart once user is auth'ed
User can update quantity using increment/decrement functionality
Kebab menu should reuse options from existing Venia mini-cart
Open Questions:

What product attributes should be listed in the view? For now, it's just size/color in the mockup. As discussed in grooming, all product options should be displayed for now.

Related Issue

[PWA-238] Product Listing (Cart)

Acceptance

Verification Stakeholders

Specification

Verification Steps

Data fetch policy I used for ease of development has been removed so its not accidentally left in once all cart features are completed. You'll need to clear your Apollo cache if you make any cart mutations outside of this view (ie. Add to Cart).

Navigate to /cart route, verify empty cart message is shown
Add Products to your cart, both simple and configurable
Go back to /cart route, verify details are displayed and aligned with mocks
Switch to mobile view and verify layout collapses and adheres to mocks

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 · 2020-01-10T19:49:03Z

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

Generated by 🚫 dangerJS against 11576c7

- Leverage fragments to auto update the cache

sirugh · 2020-01-13T20:17:35Z

packages/peregrine/lib/talons/ProductListing/useProduct.js

    const handleRemoveFromCart = useCallback(() => {
+        setIsRemoving(true);


You may want to set this to false in a catch incase removeItem fails for whatever reason.

Fixed in 0ac27f8. I need to do some validation, but I'm banking on Apollo running the refetch on error, which would make this unnecessary; but if not, would put this in an odd state. Will validate that assumption, and manually trigger a refetch onError if needed.

sirugh · 2020-01-13T20:25:12Z

packages/peregrine/lib/talons/ProductListing/useProduct.js

+            variables: {
+                cartId,
+                itemId: item.id
+            }


Depending on the order of the merges someone should add refetchQueries here and use this exported query so the order summary gets updated.

OR you could use manually update. Both should work, but the refetchQueries is probably easiest.

import PriceSummary from './priceSummary; removeItem({ variables, refetchQueries: [{ query: PriceSummary.queries.GET_PRICE_SUMMARY, variables: { cartId }, }], });

Added in 0ac27f8 (and I'll fix again once your PR is merged 😉 )

OK, I believe price summary was merged :D

packages/peregrine/lib/talons/ProductListing/useProductListing.js

sirugh · 2020-01-13T20:32:28Z

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

@@ -3,6 +3,7 @@ import React, { useMemo } from 'react';
 import { useCartPage } from '@magento/peregrine/lib/talons/CartPage/useCartPage';

 import Button from '../Button';
+import ProductListing from '../ProductListing';


I colocated the PriceSummary component with cartPage. Do you think it is likely that something else will need to use the component?

This has a public/private API implication, but I'm okay moving if we're not considering these components public API (suppose we could always export in index to denote they are public).

Maybe it's CartPage that should actually just be a RootComponent and these should all just be top-level components. Hmm.

Maybe it's CartPage that should actually just be a RootComponent and these should all just be top-level components. Hmm.

We're moving away from RootComponents being anything but shells bound to a route. I would prefer that CartPage and all pages that actually have DOM, style, talons, etc., be in components. See #1953.

Nested under CartPage in 0ac27f8

sirugh · 2020-01-14T17:44:58Z

packages/venia-ui/lib/components/ProductListing/__tests__/product.spec.js

+import { createTestInstance } from '@magento/peregrine';
+
+import Product from '../product';
+


You should also mock classify so the classnames are not undefined in the snapshots.

Mocked in 0ac27f8

sirugh · 2020-01-14T18:40:32Z

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

+            <span className={classes.name}>{name}</span>
+            <ProductOptions
+                options={options}
+                classes={{ options: classes.options, optionLabel: {} }}


An empty object is stringified to [object Object] in the html. You should just create an empty .optionLabel class in the css and pass it here.

I know this API isn't pretty, but props for using it. At some point we'll make a better solution for this use case.

I made this change in 0ac27f8, but vscode linting is complaining about the empty rule now. Is that something we enforce?

No, VSC's linter is not something we care about, so an empty rule is fine. That said, you may safely delete empty rules. The component should not throw an error if you try to apply classes.foo but there is no .foo {} defined in the CSS module.

sirugh · 2020-01-14T18:41:43Z

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

+                    text="Move to favorites"
+                    onClick={handleToggleFavorites}
+                    icon="Heart"
+                    isFilled={false}


Favorites doesn't do anything right now but I think this should use a local state variable at least.

Maybe say "wishlist" in code (if not labels too) since that lines up to Magento's terminology.

I think for now Tommy is using the existing options kebab which hardcodes the label.

I'm going to stick with the copy in the mocks, as I think this functionality is intended to be different from the auth'd wishlist feature. I'll make it usable client-side for @sirugh though 🤗

Made this functional client-side in 0ac27f8

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

sirugh · 2020-01-14T18:52:06Z

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

+// Export query to be used by other components that may need to trigger
+// a full re-fetch.
+export const GET_PRODUCT_LISTING = gql`
+    query getProductListing($cartId: String!) {


I have queries exported as a static member like fragments. Thoughts on doing that instead of exporting a const here?

ProductListing.queries = { getProductListing: gql`...` } ``

Discussed IRL, this is now obsolete.

brendanfalkowski · 2020-01-14T20:20:37Z

Just a note on the ProductListing naming. This seems a little confusing to me coming from a Magento background where "product grid" and "product list" are general terms applicable to a view mode of a category/search listing of products.

Since this component is mostly useful within cart (err, maybe within a wishlist too?) I would probably rename it CartList. We have all four of those to support, and each has different UI structure so just thinking ahead that this naming doesn't stretch too cleanly (for us). But maybe it could for simpler sites.

jimbo · 2020-01-14T23:57:51Z

packages/peregrine/lib/talons/ProductListing/useProduct.js

+    return {
+        handleEditItem,
+        handleRemoveFromCart,
+        handleToggleFavorites: () => {},


This is extremely minor, but inline functions like this are recreated on every call/render, so if something does use it, it'll re-render all the time. Might as well define const noop = () => {} outside of the component.

By request of @sirugh, I made this functional client-side in 0ac27f8, which resolves this render problem. Good to know though 😄

jimbo · 2020-01-14T23:59:01Z

packages/peregrine/lib/talons/ProductListing/useProduct.js

+    } = item;
+
+    const { price } = prices;
+    const { value: unitPrice, currency } = price;


Good rename. 👍

packages/peregrine/lib/talons/ProductListing/useProductListing.js

jimbo · 2020-01-15T00:01:46Z

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

@@ -3,6 +3,7 @@ import React, { useMemo } from 'react';
 import { useCartPage } from '@magento/peregrine/lib/talons/CartPage/useCartPage';

 import Button from '../Button';
+import ProductListing from '../ProductListing';


Maybe it's CartPage that should actually just be a RootComponent and these should all just be top-level components. Hmm.

We're moving away from RootComponents being anything but shells bound to a route. I would prefer that CartPage and all pages that actually have DOM, style, talons, etc., be in components. See #1953.

jimbo · 2020-01-15T00:11:49Z

packages/venia-ui/lib/components/ProductListing/product.css

+        'image name quantity kebab'
+        'image options quantity kebab'
+        'image price quantity kebab';
+    grid-template-columns: 100px 2fr max-content min-content;


Suggested change

grid-template-columns: 100px 2fr max-content min-content;

grid-template-columns: 100px 1fr max-content min-content;

If you only have one flexible column, 1fr is enough.

Fixed in 0ac27f8

jimbo · 2020-01-15T00:12:47Z

packages/venia-ui/lib/components/ProductListing/product.css

+    background-color: rgb(var(--venia-grey));
+    border: solid 1px rgb(var(--venia-border));
+    border-radius: 2px;
+    height: 100px;


If it's for an image, I think it's fine.

jimbo · 2020-01-15T00:14:36Z

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

+            <span className={classes.name}>{name}</span>
+            <ProductOptions
+                options={options}
+                classes={{ options: classes.options, optionLabel: {} }}


I know this API isn't pretty, but props for using it. At some point we'll make a better solution for this use case.

jimbo · 2020-01-15T00:16:14Z

Just a note on the ProductListing naming. This seems a little confusing to me coming from a Magento background where "product grid" and "product list" are general terms applicable to a view mode of a category/search listing of products.

Since this component is mostly useful within cart (err, maybe within a wishlist too?) I would probably rename it CartList. We have all four of those to support, and each has different UI structure so just thinking ahead that this naming doesn't stretch too cleanly (for us). But maybe it could for simpler sites.

Agree on all points. 👍

tjwiebell · 2020-01-15T21:41:37Z

Just a note on the ProductListing naming. This seems a little confusing to me coming from a Magento background where "product grid" and "product list" are general terms applicable to a view mode of a category/search listing of products.

Since this component is mostly useful within cart (err, maybe within a wishlist too?) I would probably rename it CartList. We have all four of those to support, and each has different UI structure so just thinking ahead that this naming doesn't stretch too cleanly (for us). But maybe it could for simpler sites.

@brendanfalkowski - I've got two suggestions related to naming and location that conflict. If I take @sirugh's advice and nest as a sub-component under CartPage, would that remove the overlap with existing Magento jargon? eg. CartPage > ProductListing.

brendanfalkowski · 2020-01-15T22:16:40Z

@tjwiebell — That's nicer from my POV. I still prefer even more explicit naming all the way down like CartPage > CartList. I've been on BEM naming for 9 years so I'm guilty as charged.

It's nice in an IDE being able to fuzzy search when the file name, component name, and outer context all have the same meta cues in naming. Said another way, I'd never search "productlist" to pick from product lists in multiple contexts when I'm really thinking "the product list in the cart" — hence "cartlist" is what I'd type and name it.

That "seems" to work better when we don't have generically named sub-components (ex: Thing/button, Thing/form, etc) vs (Thing/thingForm, Thing/thingButton, etc). Maybe the fuzzy sort puts more weight on "file name matches" than "in the path" matches. It seems to always find the exact file I meant without confusing paths that happen to match.

- Replace fragment usage in mutation to refetchQueries - Make a sub-component of CartPage - Fixup tests

…ing-v2

sirugh · 2020-01-16T15:23:21Z

packages/peregrine/lib/talons/CartPage/PriceSummary/usePriceSummary.js

@@ -25,10 +25,7 @@ export const usePriceSummary = props => {
    const [{ cartId }] = useCartContext();

    const [fetchPriceSummary, { error, loading, data }] = useLazyQuery(
-        props.query,
-        {
-            fetchPolicy: 'no-cache'


You removed this cause refetch refreshes this data right? Something is wrong -- I'm not seeing any items or a price summary after adding items to the cart.

Correct. Since I didn't know where to track the remaining work for making sure aggressive fetch policies are removed on completion, I just removed them entirely. You'll need to delete the apollo-cache entry in local storage and refresh, and you should see live data.

This is on a fresh start, cleared cache and everything.

If you start from the /cart page I believe it never is updated after future operations.

After discussion we have opened a ticket to track the ideal state work.

sirugh · 2020-01-16T15:37:47Z

packages/peregrine/lib/talons/CartPage/ProductListing/useProductListing.js

+
+    const [{ cartId }] = useCartContext();
+
+    const { data, error, loading } = useQuery(query, {


You're going to want to change this to a lazy query since cartId can change. Will this work if you sign out/sign in?

However, I'm not sure if that's necessary as cartId should update and the useQuery hook should update appropriately...

Ok, so this will work correctly IF cartId is present, but you do need to make sure you aren't calling the query when cartId is undefined. After adding an item, go to the page and sign in/out. You should see a console error for graphql related to missing cartId.

I was comfortable having error state === empty cart state, but agree its best to save the network request we know will fail. Added in f622664.

packages/peregrine/lib/talons/CartPage/ProductListing/useProductListing.js

sirugh · 2020-01-16T16:27:24Z

packages/peregrine/lib/talons/CartPage/ProductListing/useProduct.js

+            refetchQueries: [
+                {
+                    query: refetchCartQuery,
+                    variables: { cartId }


We talked offline about this -- this kinda sucks because this is sort of a magical coupling between query/variables. This component doesn't actually know what variables these queries expect. 🤷‍♂

sirugh

A few last comments.

…ing-v2

- Update tests

sirugh · 2020-01-16T19:26:58Z

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

+    const classes = mergeClasses(defaultClasses, props.classes);
+
+    if (isLoading) {
+        return <LoadingIndicator>{`Fetching Cart...`}</LoadingIndicator>;


Do we want a loading indicator here? I thought we were trying to get rid of them -- I just display nothing in summary when loading.

It's really a case-by-case thing. You're right, we don't want loading indicators everywhere, but we also don't want to have a whole view suddenly just pop in. Ultimately, the way for us to avoid having spinners everywhere will have to be something like art direction but for loading states; until we have that, having an indicator here is fine.

sirugh

Nice work. There's a lot of good stuff in here, specifically around the interaction between components/graphql/cache. Lets get this merged!

@soumya-ashok, @schensley please give this a once-over with Tommy, or use the link in the PR to check it out yourself.

@dpatil-magento you can QA this but wait to merge until @soumya-ashok or @schensley gives this a thumbs up.

…ing-v2

sirugh · 2020-01-17T19:29:04Z

packages/peregrine/lib/talons/CartPage/ProductListing/useProduct.js

+    }, []);
+
+    const handleRemoveFromCart = useCallback(async () => {
+        setIsRemoving(true);


You . may not have to use a local state. The removeItem mutation returns an object with a loading prop. I'm not sure if it waits for refetchQueries though.

- Update test snapshots with new element

dpatil-magento · 2020-01-21T18:24:44Z

QA Pass, waiting for reviewer approval on latest commits.

jimbo · 2020-01-21T18:48:12Z

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

+    const classes = mergeClasses(defaultClasses, props.classes);
+
+    if (isLoading) {
+        return <LoadingIndicator>{`Fetching Cart...`}</LoadingIndicator>;


It's really a case-by-case thing. You're right, we don't want loading indicators everywhere, but we also don't want to have a whole view suddenly just pop in. Ultimately, the way for us to avoid having spinners everywhere will have to be something like art direction but for loading states; until we have that, having an indicator here is fine.

schensley · 2020-01-21T22:23:43Z

@tjwiebell the UI looks good. The 'adjust quantity control' is yet to be implemented at this time, but the Product Listing itself is approved by UX.

tjwiebell added 3 commits January 9, 2020 10:37

Create basic stub of new product list layout

b0b5547

Get configurable mocked data working before hooking up network calls

d1f9eac

Hook everything up with data fetches

0536999

tjwiebell added in progress version: Minor This changeset includes functionality added in a backwards compatible manner. labels Jan 10, 2020

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

brendanfalkowski mentioned this pull request Jan 10, 2020

[feature]: Implement quantity dropdown as increment decrements with text-box on PDP #1281

Closed

6 tasks

tjwiebell added 4 commits January 10, 2020 17:15

- Add remove item logic

d96a174

- Leverage fragments to auto update the cache

Give item removal a mask while in flight

be47ed9

ProductListing tests

40cfacb

Finish up unit tests for Product

9554424

tjwiebell removed the in progress label Jan 14, 2020

sirugh requested changes Jan 14, 2020

View reviewed changes

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

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

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

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

jimbo reviewed Jan 15, 2020

View reviewed changes

m2-community-project bot assigned jimbo Jan 15, 2020

- Handle item remove error state

0ac27f8

- Replace fragment usage in mutation to refetchQueries - Make a sub-component of CartPage - Fixup tests

tjwiebell added 2 commits January 15, 2020 16:56

Merge remote-tracking branch 'origin/develop' into tommy/product-list…

23c60b9

…ing-v2

Fixup some component interactions after mainline merge

058a2a5

sirugh reviewed Jan 16, 2020

View reviewed changes

packages/peregrine/lib/talons/CartPage/ProductListing/useProductListing.js Show resolved Hide resolved

Re-add fetch policies as TODO items to be removed later

ea3fcf2

sirugh reviewed Jan 16, 2020

View reviewed changes

sirugh requested changes Jan 16, 2020

View reviewed changes

tjwiebell added 2 commits January 16, 2020 11:15

Merge remote-tracking branch 'origin/develop' into tommy/product-list…

324bbb9

…ing-v2

- Validate cart id before fetching data

f622664

- Update tests

sirugh reviewed Jan 16, 2020

View reviewed changes

sirugh previously approved these changes Jan 16, 2020

View reviewed changes

Merge remote-tracking branch 'origin/develop' into tommy/product-list…

7c22fee

…ing-v2

sirugh reviewed Jan 17, 2020

View reviewed changes

- Make detail content flow better based on UX feedback

05e9e13

- Update test snapshots with new element

tjwiebell dismissed sirugh’s stale review via 05e9e13 January 17, 2020 21:49

jimbo and others added 3 commits January 17, 2020 16:32

Adjust product details line-height

86234c0

Merge branch 'develop' into tommy/product-listing-v2

924b566

Merge branch 'develop' into tommy/product-listing-v2

11576c7

jimbo approved these changes Jan 21, 2020

View reviewed changes

dpatil-magento merged commit cacde9c into develop Jan 21, 2020

dpatil-magento deleted the tommy/product-listing-v2 branch January 21, 2020 22:24

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

		const handleRemoveFromCart = useCallback(() => {
		setIsRemoving(true);

		import { createTestInstance } from '@magento/peregrine';

		import Product from '../product';

	grid-template-columns: 100px 2fr max-content min-content;
	grid-template-columns: 100px 1fr max-content min-content;


		const [{ cartId }] = useCartContext();

		const { data, error, loading } = useQuery(query, {

[Cart v2] ProductListing Component #2094

[Cart v2] ProductListing Component #2094

Conversation

tjwiebell commented Jan 10, 2020 • edited Loading

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jan 10, 2020 • edited Loading

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

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

brendanfalkowski commented Jan 14, 2020 • edited Loading

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

jimbo commented Jan 15, 2020

tjwiebell commented Jan 15, 2020

brendanfalkowski commented Jan 15, 2020 • edited Loading

sirugh Jan 16, 2020 • edited Loading

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sirugh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dpatil-magento commented Jan 21, 2020

Choose a reason for hiding this comment

schensley commented Jan 21, 2020

tjwiebell commented Jan 10, 2020 •

edited

Loading

PWAStudioBot commented Jan 10, 2020 •

edited

Loading

brendanfalkowski commented Jan 14, 2020 •

edited

Loading

brendanfalkowski commented Jan 15, 2020 •

edited

Loading

sirugh Jan 16, 2020 •

edited

Loading