-
Notifications
You must be signed in to change notification settings - Fork 219
Add Order Summary card to Checkout sidebar #1959
Conversation
export { default as ProductName } from './product-name'; | ||
export { default as ProductPrice } from './product-price'; | ||
export { default as ProductSaleBadge } from './product-sale-badge'; | ||
export { default as ProductVariationData } from './product-variation-data'; |
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.
I noticed some of these blocks are very similar to the ones in atomic/components/product/
. I think it might be worth checking if they can be merged, but I left it out of this PR because it was already quite big and I don't think that's something blocking.
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.
Agree, this is good follow up for later. Great that cart & checkout are sharing these components now though :)
import './style.scss'; | ||
|
||
/** | ||
* Returns a low stock badge for a line item. |
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.
Comment pasteo :)
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.
Removed in 4a60a67.
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.
Code looks good and testing well – I tested cart and checkout blocks with a range of products (on sale, with attributes, low stock etc). Nothing is broken in cart, and checkout looks great. Also looks reasonable on mobile 🎉
There's one blocker to fix before merge: I don't think useStoreCartItemQuantity
hook is needed in checkout, since it doesn't allow changing anything.
This is an epic PR (!), so I haven't fully reviewed all the code, but I didn't see any other issues. We will of course keep testing and iterating.
Some other things I noticed while testing, we might want to follow up separately (nothing blocking, most unrelated to this PR).
- Focus highlight on order summary "collapse" heading element has different padding/margin on right/left.
- Hard-coded text color - it would be great if we can avoid this and/or use theme colors. In my cart I'm seeing a mix of black-on-white (opinionated blocks) vs. red (storefront customiser).
- Hard error when a bad coupon is entered - I think this is already logged as an issue.
- The circled quantity numbers seem a little overkill when there's 1x of a product, I'm guessing this is the intended design. Could consider showing number only when there are multiple of the item cc @garymurray. (Note this is exacerbated by my red customisation!!)
@@ -321,7 +273,7 @@ table.wc-block-cart-items { | |||
display: inline-block; | |||
} | |||
|
|||
.wc-block-cart-item__sale-badge { | |||
.wc-block-sale-badge { |
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.
Should some of these styles be in the components directly? Probably not a big deal, there's a little mixing of concerns here - the cart is overriding/tweaking component styles.
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.
I don't think so, I left them here in purpose. Even though they refer to the ProductSaleBadge
component, they are only relevant under the Cart block, so I think they belong to the Cart CSS.
<div className="wc-block-order-summary-item__total-price"> | ||
<ProductPrice | ||
currency={ currency } | ||
value={ purchasePrice * quantity } |
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.
Does this work correctly with the different tax options?
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.
I think so, those prices honor the tax settings.
I found an issue, though: #1963. But since it's affecting the Cart too, I don't think it should block this PR.
variation, | ||
} = cartItem; | ||
|
||
const { quantity } = useStoreCartItemQuantity( cartItem ); |
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.
I don't think you need this hook - you can just accept a static quantity prop from parent component (which should have all the item details).
useStoreCartItemQuantity
hook is for changing quantity or removing item :)
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.
Oh, good catch. I missed that cartItem
has the quantity
attribute. Fixed in 87214e3.
@Aljullu One more thing .. is it possible to give the description / summary text more room? They all wrap and it looks like this is unnecessary for the data I have. (e.g. |
@haszari Let's leave that as is for now. Any changes here can be made later in future iterations. |
741c5ef
to
7d4fd6e
Compare
Fixed in 7d4fd6e.
That's a tricky issue that I would like to handle in another PR, the hardcoded colors are there because in the designs they differ from the default color. I also noticed the Cart block completely ignores the Text color preference, while the Checkout block applies it inconsistently. I created an issue to discuss what's the best solution for this: #1964.
Right, I think that's not related to this PR.
Good idea. I refactored the CSS in c575d54. |
Closes #1486.
Closes #1304.
Screenshots
How to test the changes in this Pull Request: