-
Notifications
You must be signed in to change notification settings - Fork 53
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
Feat: Add Price with taxes feature #2311
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
821f70c
to
43c1248
Compare
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
420c0bb
to
71b9a5f
Compare
71b9a5f
to
3984dfb
Compare
f6098c5
to
ff213f0
Compare
a2fc06b
to
486400d
Compare
} | ||
|
||
if (isOrderFormItem(root)) { | ||
return withTax(root.sellingPrice / 1e2 / root.unitMultiplier, root.tax / 1e2, root.unitMultiplier) |
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.
The sellingPrice
is the only price that is returned already multiplied by the unitMultiplier. To ensure consistency, this value is being divided by the unitMultiplier
to return the unit price, like the others prices.
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, I just left some suggestions
packages/components/src/molecules/OrderSummary/OrderSummary.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/molecules/ProductCard/ProductCardContent.tsx
Outdated
Show resolved
Hide resolved
packages/components/src/molecules/ProductCard/ProductCardContent.tsx
Outdated
Show resolved
Hide resolved
packages/core/src/components/cart/OrderSummary/OrderSummary.tsx
Outdated
Show resolved
Hide resolved
packages/core/src/components/cart/OrderSummary/OrderSummary.tsx
Outdated
Show resolved
Hide resolved
4c80c48
to
1587ae5
Compare
6c85e7f
to
5996035
Compare
packages/core/src/components/product/ProductCard/ProductCard.tsx
Outdated
Show resolved
Hide resolved
@@ -11,6 +11,10 @@ type StoreAggregateOffer { | |||
""" | |||
lowPrice: Float! | |||
""" | |||
Lowest price among all sellers with current Taxes. |
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 (nit): just to match the descriptions you added on other files.
Lowest price among all sellers with current Taxes. | |
Lowest price among all sellers with current taxes. |
packages/components/src/molecules/OrderSummary/OrderSummary.tsx
Outdated
Show resolved
Hide resolved
*/ | ||
includeTaxes?: boolean | ||
/** | ||
* Label for determine if the price are with taxes included. |
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.
* Label for determine if the price are with taxes included. | |
* Label to determine if the price is with taxes included. |
@@ -105,6 +117,17 @@ export const StoreOffer: Record<string, Resolver<Root>> = { | |||
|
|||
return null | |||
}, | |||
listPriceWithTaxes: (root) => { | |||
if (isSearchItem(root)) { | |||
return withTax(root.ListPrice, root.Tax, root.product.unitMultiplier) |
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.
question: on listPrice
it's used root.ListPrice ?? 0
, isn't the ?? 0
necessary 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.
To be honest, I just reproduced the current listPrice
implementation. It looks like the search API could return undefined if the listPrice does not exist, but the order form always returns a value, even if it is 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.
Last Preview Link
}, | ||
highPrice: (offers) => getHighPrice(offers), | ||
lowPrice: (offers) => getLowPrice(offers), | ||
lowPriceWithTaxes: (offers) => getLowPrice(offers, { includeTaxes: true }), |
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.
As we discussed, this field doesn't exist in the https://schema.org/AggregateOffer
, and it is passing the includeTaxes: true
fixed. Is this the case for getting this value from the catalog instead of fixing it and calculating the value in the lowPrice
field directly? Also, I think there is a field to recognize taxes AggregateOffer > priceSpecification > https://schema.org/valueAddedTaxIncluded.
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.
This { includeTaxes: true } is used only inside the getLowPrice function. It's a way to reuse the function to calculate prices with and without taxes. The tax value comes from the Catalog, but whether the store will use taxes and in which section the taxes will be applied is defined in the hCMS. Because of this, both values are returned, and the front end chooses, based on the hCMS, which value should be used.
@@ -78,7 +79,18 @@ export const StoreOffer: Record<string, Resolver<Root>> = { | |||
} | |||
|
|||
if (isOrderFormItem(root)) { | |||
return root.sellingPrice / 1e2 | |||
return root.sellingPrice / 1e2 / root.unitMultiplier |
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.
Question: why are we changing the current price logic? Would this affect the current stores using the price field? c.c. @lariciamota
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.
This is a modification in the Packing feature. I discovered during this task that the sellingPrice is the only price that is already multiplied by the unitMultiplier. Because of this, some inconsistencies are happening, like a spotPrice being greater than the listPrice due to the multiplication.
This modification should only affect stores that utilize the unitMultiplier, which, at this time, is not being used by any stores.
formatter={useFormattedPrice} | ||
{...ProductPrice.props} | ||
/> | ||
<div data-fs-product-details-values-wrapper> |
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.
This can change the CSS cascade and affect styles from existing stores; nothing critical, but it could happen 👀 . @renatamottam WDYT?
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.
How are you tracking this kind of modifications side effects?
{...ProductPrice.props} | ||
/> | ||
{taxesConfiguration?.usePriceWithTaxes && ( | ||
<UILabel data-fs-product-details-taxes-label> |
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 this be overridable using section override v2 feature? maybe in the future?
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.
Yes, We can do this. Is there documentation on how to create a new section override?
--fs-order-summary-taxes-label-color : var(--fs-color-info-text); | ||
--fs-order-summary-taxes-text-size : var(--fs-text-size-tiny); | ||
--fs-order-summary-taxes-text-weight : var(--fs-text-weight-regular); |
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 we add the new tokens in the docs?
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.
Yes, I discussed this with @renatamottam. There is a task to create the documentation after the feature is merged.
--fs-product-card-taxes-label-color : var(--fs-color-info-text); | ||
--fs-product-card-taxes-text-size : var(--fs-text-size-tiny); | ||
--fs-product-card-taxes-text-weight : var(--fs-text-weight-regular); |
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 we add the new tokens in the docs?
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.
Same here
What's the purpose of this pull request?
This PR aims to add the functionality of Price with Taxes, the new feature introduced as part of the Faststore B2B initiative. This feature allows the store shows the product price with taxes included. To learn more about the Price with Taxes and the Faststore B2B initiative, please refer to the RFCs and Product Vision documentation.
B2B Faststore Product Vision
B2B Faststore RFC
Price With Taxes RFC
How it works?
PriceWithTaxes API fields
This pull request (PR) introduces new fields to the Queries
StoreOffer
(priceWithTaxes
andlistPriceWithTaxes
) andStoreAggregateOffer
(lowPriceWithTaxes
). These fields return various price types with taxes included.These fields will only be displayed if configured for each component in the Headless CMS.
Headless CMS Configuration
To ensure granularity and easy customization of the "price with taxes" feature, fields have been added to the configuration of each section in the Headless CMS. These fields determine whether a particular section should directly use price data with taxes included and specify the label text indicating that the price already includes taxes (the default value for the label is "Tax Included").
The sections that received this configuration in the CMS are:
ProductDetails
,CartSidebar
,ProductTiles
,ProductShelf
,CrossSellingProductShelf
, andProductGallery
.Taxes Configuration in CMS example
Taxes Label
Based on the Figma prototype, labels were added to components that display prices, indicating that those prices include taxes. Labels were added to the
ProductCard
andOrderForm
components, as well as to theProductDetails
section.Packing experience adjusts
During the development of the Price With Taxes feature, some improvements were identified for the Packing feature. These improvements are described throughout the PR.
How to test it?
Starters Deploy Preview
The Price with Taxes feature is being used in this preview on b2bfaststoredev preview linked to this PR. In this preview, a 30% tax has been applied to all products. This tax should be applied across the entire store. For comparison, you can use the current b2bfaststoredev link.
References
B2B Faststore Product Vision
B2B Faststore RFC
Price With Taxes RFC
Preview
PR b2bfaststoredev
Related Tasks
B2BTEAM-1643
B2BTEAM-1647