-
Notifications
You must be signed in to change notification settings - Fork 682
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
New MiniCart #2494
Changes from 5 commits
8b87781
cefae10
502ae39
50ddb0f
631e7bf
c23a972
71f9f4e
f2f2a2f
4788151
6524bd7
9975569
29cb5be
18d615e
1bec10d
9e3683b
7745326
87aa60f
f13d4fe
16a7bd7
787ca5a
7d529f6
535485c
decd9ac
2ae1589
402f24e
c2a5741
49af31a
a0a9148
a4cf8a0
99a7a7c
48203dd
74c3c12
e66bb22
96c0e4e
30edf58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,13 @@ | ||
import { useCallback } from 'react'; | ||
|
||
export const useMiniCart2 = props => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Current proposal is There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Updated to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Renamed to |
||
const { setIsOpen } = props; | ||
|
||
const onDismiss = useCallback(() => { | ||
setIsOpen(false); | ||
}, [setIsOpen]); | ||
|
||
return { | ||
onDismiss | ||
}; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,9 +1,14 @@ | ||
.root { | ||
.root_desktop { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally, we use the We also generally use the There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||
|
@@ -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
|
||
} | ||
} |
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'; | ||
|
||
|
@@ -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 | ||
}, | ||
|
@@ -32,15 +39,29 @@ const CartTrigger = props => { | |
<span className={classes.counter}>{itemCount}</span> | ||
) : null; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oops, didn't think it through - I'll change this There was a problem hiding this comment. Choose a reason for hiding this commentThe 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> | ||
); | ||
}; | ||
|
||
|
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} />; | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { default } from './minicart'; |
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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I switched to a grid There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Resolved by 2ae1589. It uses |
||||||
overflow-y: scroll; | ||||||
} | ||||||
|
||||||
.contents { | ||||||
align-content: space-between; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Generally, at least in this app, we try to avoid 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 ( There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||||||
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; | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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)); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Recall that we try to aim for integer pixel values when using
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. */ | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Eeesh, yeah good call. This is outstanding pending an alternate approach / design. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; | ||||||
} |
There was a problem hiding this comment.
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 itcart-2
for now and in the new mini cart talon we would just check fordrawer === 'cart-2'
instead of threading theisOpen
prop around.There was a problem hiding this comment.
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
)?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.