Add subtotal to cart and checkout and update the CartLineItem component with new styles #3734
Conversation
This is because we need to display the subtotal of the item AND the total (subtotal * quantity)
As per the new designs, the quantity picker should be moved below the product metadata, and the product subtotals should appear below the product name.
This is because the quantity picker is now displayed below the product metadata and name.
Size Change: -3.96 kB (0%) Total Size: 1.18 MB
ℹ️ View Unchanged
|
This is to stop product description text flowing under the total and making it look untidy.
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.
Lots of improvements in this PR, good job @opr! I left some inline comments, I think they are quite minor but posting them here to know your thoughts.
const convertedLinePrice = Dinero( { | ||
amount: parseInt( prices.raw_prices.price, 10 ), | ||
precision: parseInt( prices.raw_prices.precision, 10 ), | ||
} ).convertPrecision( currency.minorUnit ); |
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.
Are there any dangers on using .convertPrecision()
here and then using this value to calculate linePrice
? I feel we should do all calculations first and only call .convertPrecision()
when the value is going to be rendered, but not before it's used in further calculations.
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.
Good point, I have fixed this. Thanks 😸
quantity | ||
) } | ||
/> | ||
<div className="wc-block-components-order-summary-item__product-details"> |
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.
Is this div needed for something?
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 added it to allow the "left column" (product image, name, subtotal, details) to be separated from the "right column" (the total price), but I see that it's actually not necessary, thanks for pointing it out.
@@ -69,7 +73,6 @@ | |||
|
|||
.wc-block-components-order-summary-item__description { | |||
padding-left: $gap-large; | |||
padding-top: $gap; | |||
padding-bottom: $gap; | |||
line-height: 1.375; |
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.
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.
Yep, looks nicer without 👍🏻
@@ -12,12 +12,13 @@ | |||
|
|||
.wc-block-components-quantity-selector { |
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.
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.
To fix this I've removed the margin from the last-of-type
of .wc-block-components-product-details
and added margin to the bottom of .wc-block-components-product-metadata
- do you think this was the right approach? I had to add 0.75em
of margin because the Remove item text is quite small, so the gap looked bigger than the gap above.
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.
Thanks for the updates @opr! I added one last question about the details margin, but LGTM.
@@ -3,6 +3,10 @@ | |||
list-style: none; | |||
margin: 0.5em 0; | |||
padding: 0; | |||
|
|||
&:last-of-type { | |||
margin: 0; |
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'm just curious, why removing also the top margin? Shouldn't this be margin-bottom: 0
?
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.
It was just because the previous product-details
element has a margin-bottom, and so the margin-top "overlaps" and doesn't actually do anything. I figured we could save a couple of bytes by doing it this way.
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, but if there is only one product details list, then there is no margin-top, no? I think this is reproducible if you compare a variable subscription (has two details list) with a simple subscription (has one details list). The margin between the product description and the first details list will be different.
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.
Ah yea, you're correct. Thanks for pointing this out 🤦🏻♂️ fix pushed.
@opr one more thing that I completely forgot about! 🤦 We will need to update the skeleton markup. Currently when loading it shows the layout with the quantity selector in the middle column: Peek.2021-01-27.10-33.mp4The markup can be found here: |
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.
LGTM
As per the designs in this figma file: eaQcoMs7E8Ys0YgxfkOKxR-fi-0%3A1 this PR:
individual total * quantity
) to its own "column" on the sidebar.I have added some design changes here because adding the subtotal below the item name without making the other design changes caused this to look very messy.
Fixes #3629
Fixes #3631
Accessibility
Screenshots
Cart
Before
After
Checkout
Before
After
How to test the changes in this Pull Request:
item price * quantity
).Changelog