New MiniCart #2494

supernova-at · 2020-06-17T17:44:09Z

Description

This PR introduces a skeleton for the new MiniCart.

The new MiniCart's display is controlled by the CartTrigger button in the site header.
On Desktop, the CartTrigger will show this new MiniCart.
On mobile, the CartTrigger will take the user directly to the Cart page.

Once the MiniCart is visible, clicking anywhere outside of it will dismiss it.

It also:

Renames the original MiniCart to LegacyMiniCart
Creates a new Storybook entry in the Venia category

Follow up tasks in the new MiniCart epic will implement this component's actual functionality.

Related Issue

Closes PWA-600.

Acceptance

Verification Stakeholders

@jimbo

Specification

Verification Steps

On Desktop

Add an item to your cart
Verify that the MiniCart no longer automatically appears
Click the CartTrigger (shopping bag icon in the site header)
Verify that the new MiniCart skeleton appears
Verify that clicking outside of the MiniCart dismisses it

On Mobile

Click the CartTrigger (shopping bag icon in the site header)
Verify that you are directed to the new Cart page
Verify that the new MiniCart does not appear

In Storybook

yarn workspace @magento/venia-ui run storybook
Verify a new "MiniCart" entry appears in the "Venia"-specific category

Screenshots / Screen Captures (if appropriate)

Checklist

I have added tests to cover my changes, if necessary.

I have updated the documentation accordingly, if necessary.

desktop vs mobile

to toggle the cart drawer

PWAStudioBot · 2020-06-17T17:54:05Z

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

Generated by 🚫 dangerJS against 30edf58

devops-pwa-codebuild · 2020-06-17T18:03:52Z

Performance Test Results

The following fails have been reported by WebpageTest. These numbers indicates a possible performance issue with the PR which requires further manual testing to validate.

https://pr-2494.pwa-venia.com : LH Performance Expected 0.85 Actual 0.52, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 90 Actual 83.333333333333
https://pr-2494.pwa-venia.com/venia-tops.html : LH Performance Expected 0.75 Actual 0.36, LH Best Practices Expected 1 Actual 0.92
https://pr-2494.pwa-venia.com/valeria-two-layer-tank.html : LH Performance Expected 0.8 Actual 0.42, LH Accessibility Expected 0.9 Actual 0.89, LH Best Practices Expected 1 Actual 0.92, WPT Cache Expected 65 Actual 50

sirugh

Just some comments - not a formal review.

packages/peregrine/lib/talons/Header/useCartTrigger.js

packages/peregrine/lib/talons/ProductFullDetail/useProductFullDetail.js

packages/venia-ui/lib/components/MiniCart2/minicart.js

packages/venia-ui/lib/components/Header/cartTrigger.js

…dio into supernova/600_minicartv2

supernova-at · 2020-06-18T15:00:08Z

PR Updated:

Added the white background behind the cart trigger when the minicart is open to match the mocks
Blurred out the background instead of graying it out

packages/venia-ui/lib/components/MiniCart2/minicart.css

sirugh

Reviewed!

packages/venia-ui/lib/components/MiniCart2/minicart.css

packages/venia-ui/lib/components/Header/cartTrigger.css

packages/venia-ui/lib/components/MiniCart2/minicart.css

sirugh · 2020-06-18T17:19:29Z

packages/peregrine/lib/talons/Header/useCartTrigger.js


    const [getItemCount, { data }] = useLazyQuery(getItemCountQuery, {
        fetchPolicy: 'cache-and-network'
    });
    const [fetchCartId] = useMutation(createCartMutation);
    const fetchCartDetails = useAwaitQuery(getCartDetailsQuery);

+    const [miniCartIsOpen, setMiniCartIsOpen] = useState(false);


What do we gain from using local state? I think we can still get away with using toggleDrawer. You could even just call it cart-2 for now and in the new mini cart talon we would just check for drawer === 'cart-2' instead of threading the isOpen prop around.

toggleDrawer is app-wide / global state and originally designed so that only one drawer was open at a time. Dialog introduced the concept of these components doing their own masking (as opposed to the app itself having to do it), so I was running with that.

Basically, I am intentionally trying to bring the state local here - aren't we trying to get away from the site-wide contexts (useAppContext::toggleDrawer)?

Basically, I am intentionally trying to bring the state local here - aren't we trying to get away from the site-wide contexts (useAppContext::toggleDrawer)?

We're avoiding global state for things that aren't global state. If the new mocks call for multiple drawers being opened at once then this would no longer fit into the old drawer pattern. So that's fine.

packages/venia-ui/lib/components/App/app.js

sirugh · 2020-06-18T17:23:24Z

packages/venia-ui/lib/components/MiniCart2/minicart.css

+}
+
+.body {
+    margin-top: 1rem;


This is adding some weird whitespace above the body. I do think using top spacing is correct. Not sure what to do here:

I switched to a grid gap in 9e3683b. There's still that whitespace but at least it is consistent on the top and bottom now. You're right, I'm not sure what to do about this.

Resolved by 2ae1589. It uses padding now instead, which allows the contents to scroll into the whitespace.

packages/venia-ui/lib/components/Header/cartTrigger.css

jimbo · 2020-06-22T16:23:12Z

packages/peregrine/lib/talons/MiniCart2/useMiniCart2.js

@@ -0,0 +1,13 @@
+import { useCallback } from 'react';
+
+export const useMiniCart2 = props => {


I think we should come up with a real name for this component.

Current proposal is ShoppingBag, which is also a term we can begin to use in place of "minicart" more generally.

Updated to ShoppingBag in 6524bd7.

Renamed to MiniCart in 535485c.

jimbo · 2020-06-22T17:18:08Z

packages/venia-ui/lib/components/Header/cartTrigger.css

@@ -1,9 +1,29 @@
-.root {
+.root_desktop {


Generally, we use the .root classname for the component's root element. Here, the root is a fragment, so we wouldn't really have a root.

We also generally use the x_y format to indicate that y is one state of x. Here, root_desktop and root_mobile are sibling elements, so we should probably not use that format here. Further, desktop and mobile don't really get at the heart of the distinction here, so perhaps trigger (desktop, triggers a dropdown) and link (mobile, links to a new page) would distinguish the two elements better.

Resolved by 29cb5be.

jimbo · 2020-06-22T17:20:43Z

packages/venia-ui/lib/components/Header/cartTrigger.css

+    display: none;
+}
+
+.background {


We've had a few conversations about this in Slack, but I'd like to avoid creating (and naming) elements just for presentational quirks. In this case, since I anticipated this situation, I've been pushing back against the design calling for a background on the trigger. Can we find a different way to highlight the trigger when the dropdown is active?

This is still outstanding pending an alternate approach / design.

Resolved with a new design in f13d4fe.

packages/venia-ui/lib/components/Header/cartTrigger.css

jimbo · 2020-06-22T17:29:31Z

packages/venia-ui/lib/components/MiniCart2/minicart.css

+}
+
+.mask {
+    /* The mask takes up the entire screen. */


I believe the design calls for the mask to obscure the body but not the header. This is a slightly complicated requirement, since shadow interactions start to matter.

Also, keep in mind that clicking the header—or the trigger a second time—must close the dropdown if it's open. The implementation here (clicking on the mask) relies on the mask overlaying the header, so it may need a new approach.

Eeesh, yeah good call. This is outstanding pending an alternate approach / design.

Resolved with the new design in f13d4fe. There is no mask anymore.

jimbo · 2020-06-22T17:31:36Z

packages/venia-ui/lib/components/MiniCart2/minicart.css

+}
+
+.footer {
+    border-top: solid 0.4rem rgb(var(--venia-grey));


Recall that we try to aim for integer pixel values when using rem units. Here, though, I'd like to stick to a 2px border; the design's thick border here seems inconsistent with the rest of our design.

Suggested change

border-top: solid 0.4rem rgb(var(--venia-grey));

border-top: 2px solid rgb(var(--venia-grey));

Resolved in 18d615e.

jimbo · 2020-06-22T17:35:05Z

packages/venia-ui/lib/components/MiniCart2/minicart.css

+    box-shadow: 1px 1px 5px rgb(var(--venia-grey-darker));
+    display: grid;
+    /* The header and footer heights may change based on PWA-116 and PWA-601. */
+    grid-template-rows: 2rem 1fr 8rem;


It seems more likely that the final row's height should depend on its content. If there's a minimum height, try minmax().

Suggested change

grid-template-rows: 2rem 1fr 8rem;

grid-template-rows: 2rem 1fr minmax(2rem, auto);

Nice. Resolved in 18d615e.

jimbo · 2020-06-22T17:40:06Z

packages/venia-ui/lib/components/MiniCart2/minicart.css

+}
+
+.contents {
+    align-content: space-between;


Generally, at least in this app, we try to avoid space-between, since it results in variable gaps.

Whenever you have a situation where the allocation of space isn't fixed, you'll have to decide whether the tracks or the gaps will be fixed in size, while the other is variable; here, we prefer fixed gaps (gap) and variable tracks.

I didn't need this after all anyway - the body track is sized at 1fr so there was no space to allocate in the first place. Removed and replaced with gap in 9e3683b.

packages/venia-ui/lib/components/MiniCart2/minicart.css

…cartv2

sirugh

Great job! I did notice that on mobile the cart trigger appears on the cart/checkout pages but doesn't do anything. Might be worth removing in those cases.

supernova-at · 2020-06-25T14:35:43Z

Might be worth removing in those cases.

I ran this by UX: consensus is it's like clicking the VENIA logo while you're on the homepage. It won't do anything but we don't want to remove it.

jimbo · 2020-06-25T17:38:52Z

packages/venia-ui/lib/components/Header/cartTrigger.js

        <span className={classes.counter}>{itemCountDisplay}</span>
    ) : null;
+    const maybeMiniCartOpenIndicator = miniCartIsOpen ? (
+        <div className={classes.openIndicator} />
+    ) : null;


We don't normally do presentational elements; ordinarily this would be a border or shadow on the button. Did you have a motive for this approach?

Oops, didn't think it through - I'll change this

Resolved by a4cf8a0.

jimbo · 2020-06-25T17:40:20Z

Thanks for going through, renaming everything, and fixing the tests. Hopefully this naming will stick. 👍

…en indicator

…cartv2

…dio into supernova/600_minicartv2

jimbo

Looks good to me. I may do some layout cleanup around the header in a separate PR, but this is good for now. 👍

dpatil-magento · 2020-07-07T14:46:46Z

@supernova-at Some conflicts to resolve and an observation on desktop >> while opening/closing mini cart, page shifts a bit to accommodate scroll bar. This might be exists on develop also but not much visible but new implementation makes it more visible.

…cartv2

supernova-at · 2020-07-07T16:44:36Z

PR Updated:

Resolved merge conflicts

an observation on desktop >> while opening/closing mini cart, page shifts a bit to accommodate scroll bar.

I cannot reproduce this, even though I can clearly see it happening in your image. What browser are you using here?

…cartv2

…dio into supernova/600_minicartv2

sirugh · 2020-07-08T15:26:07Z

@supernova-at it happens on Chrome for me but also on dev:

dpatil-magento · 2020-07-08T15:29:29Z

@supernova-at as discussed, I have created bug https://jira.corp.magento.com/browse/PWA-735 in backlog. Issue is in Chrome and Safari, Firefox works fine.

drc0 · 2020-07-09T17:23:51Z

@sirugh this could be a possible fix but this depends on the main container always pushing the footer outside the viewport so there is always a scroll bar visible: #2543

supernova-at added 4 commits June 17, 2020 11:34

MiniCart2 introduced in Storybook

8b87781

Updates cartTrigger click handler based on

cefae10

desktop vs mobile

CartTrigger controls MiniCart visibility

502ae39

Remove MiniCart from App and remove all calls

50ddb0f

to toggle the cart drawer

Merge branch 'develop' into supernova/600_minicartv2

631e7bf

PWAStudioBot added pkg:peregrine pkg:venia-ui labels Jun 17, 2020

sirugh reviewed Jun 17, 2020

View reviewed changes

m2-community-project bot assigned sirugh Jun 17, 2020

m2-community-project bot added the Progress: review label Jun 17, 2020

m2-community-project bot assigned supernova-at Jun 17, 2020

supernova-at added 2 commits June 18, 2020 09:52

Adds background lift to cart trigger when minicart is open

c23a972

Merge branch 'supernova/600_minicartv2' of github.com:magento/pwa-stu…

71f9f4e

…dio into supernova/600_minicartv2

supernova-at commented Jun 18, 2020

View reviewed changes

packages/venia-ui/lib/components/MiniCart2/minicart.css Outdated Show resolved Hide resolved

sirugh requested changes Jun 18, 2020

View reviewed changes

Fixes dimensions and mask.

f2f2a2f

supernova-at commented Jun 22, 2020

View reviewed changes

packages/venia-ui/lib/components/Header/cartTrigger.css Show resolved Hide resolved

jimbo suggested changes Jun 22, 2020

View reviewed changes

m2-community-project bot assigned jimbo Jun 22, 2020

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

supernova-at added 3 commits June 22, 2020 13:40

Merge remote-tracking branch 'origin/develop' into supernova/600_mini…

4788151

…cartv2

Renames to ShoppingBag

6524bd7

Updates CSS to use new global CSS properties

9975569

Adds unit test for MiniCart component

49af31a

sirugh previously approved these changes Jun 24, 2020

View reviewed changes

Merge branch 'develop' into supernova/600_minicartv2

a0a9148

jimbo reviewed Jun 25, 2020

View reviewed changes

supernova-at added 3 commits June 26, 2020 11:18

Uses box-shadow instead of presentational element for cart trigger op…

a4cf8a0

…en indicator

Merge remote-tracking branch 'origin/develop' into supernova/600_mini…

99a7a7c

…cartv2

Merge branch 'supernova/600_minicartv2' of github.com:magento/pwa-stu…

48203dd

…dio into supernova/600_minicartv2

supernova-at dismissed sirugh’s stale review via 48203dd June 26, 2020 16:19

jimbo previously approved these changes Jun 26, 2020

View reviewed changes

sirugh previously approved these changes Jun 26, 2020

View reviewed changes

Merge remote-tracking branch 'origin/develop' into supernova/600_mini…

74c3c12

…cartv2

supernova-at dismissed stale reviews from sirugh and jimbo via 74c3c12 July 7, 2020 16:43

Merge branch 'develop' into supernova/600_minicartv2

e66bb22

jimbo approved these changes Jul 7, 2020

View reviewed changes

supernova-at added 2 commits July 8, 2020 09:40

Merge remote-tracking branch 'origin/develop' into supernova/600_mini…

96c0e4e

…cartv2

Merge branch 'supernova/600_minicartv2' of github.com:magento/pwa-stu…

30edf58

…dio into supernova/600_minicartv2

sirugh approved these changes Jul 8, 2020

View reviewed changes

dpatil-magento merged commit ebc7b94 into develop Jul 8, 2020

dpatil-magento deleted the supernova/600_minicartv2 branch July 8, 2020 15:49

m2-community-project bot removed the Progress: review label Jul 8, 2020

drc0 mentioned this pull request Jul 9, 2020

Scroll lock effect avoid body shift when activated #2543

Merged

		@@ -0,0 +1,13 @@
		import { useCallback } from 'react';

		export const useMiniCart2 = props => {

	border-top: solid 0.4rem rgb(var(--venia-grey));
	border-top: 2px solid rgb(var(--venia-grey));

	grid-template-rows: 2rem 1fr 8rem;
	grid-template-rows: 2rem 1fr minmax(2rem, auto);

		@@ -1,9 +1,29 @@
		.root {
		.root_desktop {

New MiniCart #2494

New MiniCart #2494

Conversation

supernova-at commented Jun 17, 2020 • edited Loading

Description

Related Issue

Acceptance

Verification Stakeholders

Specification

Verification Steps

On Desktop

On Mobile

In Storybook

Screenshots / Screen Captures (if appropriate)

Checklist

PWAStudioBot commented Jun 17, 2020 • edited Loading

devops-pwa-codebuild commented Jun 17, 2020 • edited Loading

sirugh left a comment

Choose a reason for hiding this comment

supernova-at commented Jun 18, 2020

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

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 Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

sirugh left a comment • edited Loading

Choose a reason for hiding this comment

supernova-at commented Jun 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimbo commented Jun 25, 2020

jimbo left a comment

Choose a reason for hiding this comment

dpatil-magento commented Jul 7, 2020 • edited Loading

supernova-at commented Jul 7, 2020

sirugh commented Jul 8, 2020 • edited Loading

dpatil-magento commented Jul 8, 2020

drc0 commented Jul 9, 2020

supernova-at commented Jun 17, 2020 •

edited

Loading

PWAStudioBot commented Jun 17, 2020 •

edited

Loading

devops-pwa-codebuild commented Jun 17, 2020 •

edited

Loading

supernova-at Jun 23, 2020 •

edited

Loading

sirugh left a comment •

edited

Loading

supernova-at commented Jun 25, 2020 •

edited

Loading

dpatil-magento commented Jul 7, 2020 •

edited

Loading

sirugh commented Jul 8, 2020 •

edited

Loading