-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
feat(dashboard,cart,types,utils): refine order details summary #13313
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
Conversation
🦋 Changeset detectedLatest commit: c424922 The changes in this PR will be included in the next version bump. This PR includes changesets to release 71 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
The latest updates on your projects. Learn more about Vercel for GitHub.
8 Skipped Deployments
|
@@ -250,6 +243,7 @@ export function decorateCartTotals( | |||
cart.item_total = new BigNumber(itemsTotal) | |||
cart.item_subtotal = new BigNumber(itemsSubtotal) | |||
cart.item_tax_total = new BigNumber(itemsTaxTotal) | |||
cart.item_discount_total = new BigNumber(itemsDiscountTotal) |
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.
added those 2 properties. It was a comment from @fPolic and I believe it is a good addition since I will be using it in the admin dashboard + the nextjs storefront
<Text className="text-ui-fg-subtle" size="small" leading="compact"> | ||
{t("fields.creditTotal")} | ||
</Text> | ||
{getTotalCreditLines(order.credit_lines ?? []) > 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.
Is a bit confusing, so will hide if 0 as requested by Oli
/snapshot-this |
🚀 A snapshot release has been made for this PRTest the snapshots by updating your yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add [email protected] yarn add @medusajs/[email protected] yarn add [email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected] yarn add @medusajs/[email protected]
|
Please use the snapshot above to check for a few orders you might have locally. The more cases covered, the better. I have checked with taxes, without taxes, with discounts, no discounts, etc. but still. It is better to check for multiple cases across multiple projects |
...rd/src/routes/orders/order-detail/components/order-summary-section/order-summary-section.tsx
Outdated
Show resolved
Hide resolved
...rd/src/routes/orders/order-detail/components/order-summary-section/order-summary-section.tsx
Outdated
Show resolved
Hide resolved
...rd/src/routes/orders/order-detail/components/order-summary-section/order-summary-section.tsx
Outdated
Show resolved
Hide resolved
...rd/src/routes/orders/order-detail/components/order-summary-section/order-summary-section.tsx
Show resolved
Hide resolved
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.
Looks good to me overall!
nit: draft orders have similar UI, should we add a ticket to revisit that as well
Will add a ticket yes. @olivermrbl would you prefer it being part of the same project? I am playing with the draft order ui right now and there seems to be many bugs. Maybe it should be part of another project to make this feature better |
Would love to create a follow-up project to clean this up; let's sync Monday on this |
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.
Overall LGTM, few comments/suggestions.
order.items.forEach((item) => | ||
item.adjustments?.forEach((adj) => { | ||
codes.add(adj.code) | ||
const taxes = useMemo(() => { |
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.
suggestion: I wanted to suggest a minor change here; instead of breaking down taxes by type (item, shipping), should we group them by the tax rate code? This seems to be a standard and is used by other platforms. See Shopify below:

As a consequence, this would mean we don't differentiate between item and shipping tax total, but we squash these two together.
The reasoning behind this is that, as a merchant, the type of tax doesn't really matter much; what matters is that the correct tax rates were applied to the order. E.g. in NYC, the number of tax lines get pretty wild, as you can see above, and in those scenarios, giving the merchant a tax total doesn't provide much insight into why the tax total got to where it is.
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.
...rd/src/routes/orders/order-detail/components/order-summary-section/order-summary-section.tsx
Outdated
Show resolved
Hide resolved
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.
lfg, nice work!
WHAT: Small rework of the order details summary section
WHY: With the introduction of shipping promotions, it made sense to rework a little but the order details section in order to show better the discounts being applied
HOW: Reworked the sections in order to show the amounts and make sure the merchant can sum up everything and understand the totals
TESTING: Manually with a few orders covering multiple cases
CLOSES 1142