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

New MiniCart #2494

Merged
merged 35 commits into from
Jul 8, 2020
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
8b87781
MiniCart2 introduced in Storybook
supernova-at Jun 17, 2020
cefae10
Updates cartTrigger click handler based on
supernova-at Jun 17, 2020
502ae39
CartTrigger controls MiniCart visibility
supernova-at Jun 17, 2020
50ddb0f
Remove MiniCart from App and remove all calls
supernova-at Jun 17, 2020
631e7bf
Merge branch 'develop' into supernova/600_minicartv2
supernova-at Jun 17, 2020
c23a972
Adds background lift to cart trigger when minicart is open
supernova-at Jun 18, 2020
71f9f4e
Merge branch 'supernova/600_minicartv2' of github.com:magento/pwa-stu…
supernova-at Jun 18, 2020
f2f2a2f
Fixes dimensions and mask.
supernova-at Jun 22, 2020
4788151
Merge remote-tracking branch 'origin/develop' into supernova/600_mini…
supernova-at Jun 22, 2020
6524bd7
Renames to ShoppingBag
supernova-at Jun 23, 2020
9975569
Updates CSS to use new global CSS properties
supernova-at Jun 23, 2020
29cb5be
Renames CSS classes
supernova-at Jun 23, 2020
18d615e
Updates Shopping Bag CSS
supernova-at Jun 23, 2020
1bec10d
Runs prettier
supernova-at Jun 23, 2020
9e3683b
Removes margin in grid item in favor of gap in grid container
supernova-at Jun 23, 2020
7745326
Merge remote-tracking branch 'origin/develop' into supernova/600_mini…
supernova-at Jun 23, 2020
87aa60f
Removes feature flag. Adds transition animation
supernova-at Jun 24, 2020
f13d4fe
Adds shopping bag open indicator
supernova-at Jun 24, 2020
16a7bd7
Uses useDropdown
supernova-at Jun 24, 2020
787ca5a
Updates Storybook entry
supernova-at Jun 24, 2020
7d529f6
Renames MiniCart to LegacyMiniCart
supernova-at Jun 24, 2020
535485c
Renames ShoppingBag to MiniCart
supernova-at Jun 24, 2020
decd9ac
Consistency updates
supernova-at Jun 24, 2020
2ae1589
Fixes body contents scrolling to desired effect
supernova-at Jun 24, 2020
402f24e
Updates callback function names for consistency. Runs prettier
supernova-at Jun 24, 2020
c2a5741
Adds unit test for talon
supernova-at Jun 24, 2020
49af31a
Adds unit test for MiniCart component
supernova-at Jun 24, 2020
a0a9148
Merge branch 'develop' into supernova/600_minicartv2
supernova-at Jun 25, 2020
a4cf8a0
Uses box-shadow instead of presentational element for cart trigger op…
supernova-at Jun 26, 2020
99a7a7c
Merge remote-tracking branch 'origin/develop' into supernova/600_mini…
supernova-at Jun 26, 2020
48203dd
Merge branch 'supernova/600_minicartv2' of github.com:magento/pwa-stu…
supernova-at Jun 26, 2020
74c3c12
Merge remote-tracking branch 'origin/develop' into supernova/600_mini…
supernova-at Jul 7, 2020
e66bb22
Merge branch 'develop' into supernova/600_minicartv2
dpatil-magento Jul 7, 2020
96c0e4e
Merge remote-tracking branch 'origin/develop' into supernova/600_mini…
supernova-at Jul 8, 2020
30edf58
Merge branch 'supernova/600_minicartv2' of github.com:magento/pwa-stu…
supernova-at Jul 8, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions packages/peregrine/lib/talons/Header/useCartTrigger.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,12 @@
import { useCallback, useEffect } from 'react';
import { useCallback, useEffect, useState } from 'react';
import {
useApolloClient,
useLazyQuery,
useMutation
} from '@apollo/react-hooks';
import { useHistory } from 'react-router-dom';

import { useCartContext } from '@magento/peregrine/lib/context/cart';
import { useAppContext } from '@magento/peregrine/lib/context/app';
import { useAwaitQuery } from '@magento/peregrine/lib/hooks/useAwaitQuery';

export const useCartTrigger = props => {
Expand All @@ -15,15 +16,17 @@ export const useCartTrigger = props => {
} = props;

const apolloClient = useApolloClient();
const [, { toggleDrawer }] = useAppContext();
const [{ cartId }, { getCartDetails }] = useCartContext();
const history = useHistory();

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

const [miniCartIsOpen, setMiniCartIsOpen] = useState(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.


const itemCount = data ? data.cart.total_quantity : 0;

useEffect(() => {
Expand All @@ -42,16 +45,21 @@ export const useCartTrigger = props => {
}
}, [cartId, getItemCount]);

const handleClick = useCallback(async () => {
toggleDrawer('cart');
sirugh marked this conversation as resolved.
Show resolved Hide resolved
await getCartDetails({
fetchCartId,
fetchCartDetails
});
}, [fetchCartDetails, fetchCartId, getCartDetails, toggleDrawer]);
const handleDesktopClick = useCallback(async () => {
// On desktop show the minicart.
setMiniCartIsOpen(true);
}, []);

const handleMobileClick = useCallback(() => {
// On mobile send the user to the cart page.
history.push('/cart');
}, [history]);

return {
handleClick,
itemCount
handleDesktopClick,
handleMobileClick,
itemCount,
miniCartIsOpen,
setMiniCartIsOpen
};
};
13 changes: 13 additions & 0 deletions packages/peregrine/lib/talons/MiniCart2/useMiniCart2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
import { useCallback } from 'react';

export const useMiniCart2 = props => {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated to ShoppingBag in 6524bd7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to MiniCart in 535485c.

const { setIsOpen } = props;

const onDismiss = useCallback(() => {
setIsOpen(false);
}, [setIsOpen]);

return {
onDismiss
};
};
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ import { useCallback, useState, useMemo } from 'react';
import { useMutation } from '@apollo/react-hooks';
import { useCartContext } from '@magento/peregrine/lib/context/cart';

import { useAppContext } from '@magento/peregrine/lib/context/app';
import { useAwaitQuery } from '@magento/peregrine/lib/hooks/useAwaitQuery';
import { appendOptionsToPayload } from '@magento/peregrine/lib/util/appendOptionsToPayload';
import { findMatchingVariant } from '@magento/peregrine/lib/util/findMatchingProductVariant';
Expand Down Expand Up @@ -164,7 +163,6 @@ export const useProductFullDetail = props => {
productType
);

const [, { toggleDrawer }] = useAppContext();
supernova-at marked this conversation as resolved.
Show resolved Hide resolved
const [{ isAddingItem }, { addItemToCart }] = useCartContext();

const [addConfigurableProductToCart] = useMutation(
Expand Down Expand Up @@ -236,7 +234,6 @@ export const useProductFullDetail = props => {
fetchCartDetails,
fetchCartId
});
toggleDrawer('cart');
} else {
console.error('Unsupported product type. Cannot add to cart.');
}
Expand All @@ -251,8 +248,7 @@ export const useProductFullDetail = props => {
optionSelections,
product,
productType,
quantity,
toggleDrawer
quantity
]);

const handleSelectionChange = useCallback(
Expand Down
7 changes: 0 additions & 7 deletions packages/venia-ui/lib/components/App/__tests__/app.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import { useAppContext } from '@magento/peregrine/lib/context/app';

import Main from '../../Main';
import Mask from '../../Mask';
import MiniCart from '../../MiniCart';
import Navigation from '../../Navigation';
import Routes from '../../Routes';

Expand All @@ -13,7 +12,6 @@ jest.mock('../../Head', () => ({
Title: () => 'Title'
}));
jest.mock('../../Main', () => 'Main');
jest.mock('../../MiniCart', () => 'MiniCart');
jest.mock('../../Navigation', () => 'Navigation');
jest.mock('../../Routes', () => 'Routes');
jest.mock('../../ToastContainer', () => 'ToastContainer');
Expand Down Expand Up @@ -144,7 +142,6 @@ test('renders a full page with onlineIndicator and routes', () => {
const { root } = createTestInstance(<App {...appProps} />);

getAndConfirmProps(root, Navigation);
getAndConfirmProps(root, MiniCart);

const main = getAndConfirmProps(root, Main, {
isMasked: false
Expand Down Expand Up @@ -231,10 +228,6 @@ test('displays open nav or drawer', () => {
const { root: openNav } = createTestInstance(<App {...props} />);

getAndConfirmProps(openNav, Navigation);

const { root: openCart } = createTestInstance(<App {...props} />);

getAndConfirmProps(openCart, MiniCart);
});

test('renders with renderErrors', () => {
Expand Down
2 changes: 0 additions & 2 deletions packages/venia-ui/lib/components/App/app.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import { useApp } from '@magento/peregrine/lib/talons/App/useApp';
import { HeadProvider, Title } from '../Head';
import Main from '../Main';
import Mask from '../Mask';
import MiniCart from '../MiniCart';
import Navigation from '../Navigation';
import Routes from '../Routes';
import ToastContainer from '../ToastContainer';
Expand Down Expand Up @@ -96,7 +95,6 @@ const App = props => {
</Main>
<Mask isActive={hasOverlay} dismiss={handleCloseDrawer} />
<Navigation />
<MiniCart />
sirugh marked this conversation as resolved.
Show resolved Hide resolved
<ToastContainer />
</HeadProvider>
);
Expand Down
20 changes: 19 additions & 1 deletion packages/venia-ui/lib/components/Header/cartTrigger.css
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
.root {
.root_desktop {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved by 29cb5be.

composes: root from '../clickable.css';
height: 3rem;
min-width: 3rem;
}

.root_mobile {
composes: root_desktop;
display: none;
}

.counter {
font-weight: 600;
margin-left: 0.25rem;
Expand All @@ -17,3 +22,16 @@
composes: root from '../Icon/icon.css';
--fill: rgb(var(--venia-text));
}

/*
* Mobile-specific styles.
*/
@media (max-width: 960px) {
.root_desktop {
display: none;
}

.root_mobile {
display: block;
sirugh marked this conversation as resolved.
Show resolved Hide resolved
}
}
41 changes: 31 additions & 10 deletions packages/venia-ui/lib/components/Header/cartTrigger.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import React from 'react';
import React, { Fragment } from 'react';
import { shape, string } from 'prop-types';
import { ShoppingCart as ShoppingCartIcon } from 'react-feather';

Expand All @@ -8,11 +8,18 @@ import { mergeClasses } from '../../classify';
import CREATE_CART_MUTATION from '../../queries/createCart.graphql';
import GET_CART_DETAILS_QUERY from '../../queries/getCartDetails.graphql';
import Icon from '../Icon';
import MiniCart from '../MiniCart2';
import defaultClasses from './cartTrigger.css';
import { GET_ITEM_COUNT_QUERY } from './cartTrigger.gql';

const CartTrigger = props => {
const { handleClick, itemCount } = useCartTrigger({
const {
handleDesktopClick,
handleMobileClick,
itemCount,
miniCartIsOpen,
setMiniCartIsOpen
} = useCartTrigger({
mutations: {
createCartMutation: CREATE_CART_MUTATION
},
Expand All @@ -32,15 +39,29 @@ const CartTrigger = props => {
<span className={classes.counter}>{itemCount}</span>
) : null;
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved by a4cf8a0.


// Because this button behaves differently on desktop and mobile
// we render two buttons that differ only in their click handler
// and control which one displays in CSS.
return (
<button
aria-label={buttonAriaLabel}
className={classes.root}
onClick={handleClick}
>
<Icon classes={iconClasses} src={ShoppingCartIcon} />
{itemCounter}
</button>
<Fragment>
<button
aria-label={buttonAriaLabel}
className={classes.root_desktop}
onClick={handleDesktopClick}
>
<Icon classes={iconClasses} src={ShoppingCartIcon} />
{itemCounter}
</button>
<button
aria-label={buttonAriaLabel}
className={classes.root_mobile}
onClick={handleMobileClick}
>
<Icon classes={iconClasses} src={ShoppingCartIcon} />
{itemCounter}
</button>
<MiniCart isOpen={miniCartIsOpen} setIsOpen={setMiniCartIsOpen} />
supernova-at marked this conversation as resolved.
Show resolved Hide resolved
</Fragment>
);
};

Expand Down
20 changes: 20 additions & 0 deletions packages/venia-ui/lib/components/MiniCart2/__stories__/minicart.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
import React, { useState } from 'react';
import { storiesOf } from '@storybook/react';

import MiniCart from '../minicart';

const stories = storiesOf('Venia/MiniCart2', module);

/*
* Story definitions.
*/

stories.add('Default', () => {
const [isOpen, setIsOpen] = useState(true);
const props = {
isOpen,
setIsOpen
};

return <MiniCart {...props} />;
});
1 change: 1 addition & 0 deletions packages/venia-ui/lib/components/MiniCart2/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
export { default } from './minicart';
80 changes: 80 additions & 0 deletions packages/venia-ui/lib/components/MiniCart2/minicart.css
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
.root {
/* The root aside element takes up the whole screen. */
position: fixed;
left: 0;
top: 0;
height: 100%;
width: 100%;

/* It is hidden by default. */
opacity: 0;
visibility: hidden;

/* It animates to being closed. */
transition-duration: 192ms;
transition-timing-function: var(--venia-anim-out);
sirugh marked this conversation as resolved.
Show resolved Hide resolved
transition-property: opacity, visibility;

/* It sits over all background content. */
z-index: 3;
}

.root_open {
composes: root;

opacity: 1;
visibility: visible;

/* It animates to being open. */
transition-duration: 224ms;
transition-timing-function: var(--venia-anim-in);
}

.body {
margin-top: 1rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Image from Gyazo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

overflow-y: scroll;
}

.contents {
align-content: space-between;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@supernova-at supernova-at Jun 23, 2020

Choose a reason for hiding this comment

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

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.

background-color: rgb(var(--venia-background-color));
border-radius: 1px;
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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice. Resolved in 18d615e.

/* These heights may change based on PWA-605 and should be based on 1.5 and 2.5 visible items. */
height: 100%;
min-height: 16rem;
max-height: 22.5rem;
sirugh marked this conversation as resolved.
Show resolved Hide resolved
overflow: hidden;
padding: 1rem;
position: fixed;
right: 0;
/* This value is equal to the site header height. */
top: 3.5rem;
width: 30rem;
}

.footer {
border-top: solid 0.4rem rgb(var(--venia-grey));
Copy link
Contributor

Choose a reason for hiding this comment

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

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));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Resolved in 18d615e.

overflow: hidden;
}

.header {
border-bottom: solid 0.15rem rgb(var(--venia-grey));
overflow: hidden;
}

.mask {
/* The mask takes up the entire screen. */
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

position: absolute;
left: 0;
top: 0;
height: 100%;
width: 100%;

/* The mask is a semi-transparent grey. */
background-color: rgb(var(--venia-grey-darker));
opacity: 0.5;
}
Loading