-
Notifications
You must be signed in to change notification settings - Fork 125
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
[Product Tile] Promotion callout message (@W-13974753@) #1786
Conversation
|
||
// NOTE: API inconsistency - with getProduct call, a variant does not have productPromotions | ||
const promos = data.productPromotions ?? product.productPromotions ?? [] | ||
const promo = promos.find((promo) => promo.promotionalPrice === minPrice) ?? promos[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.
This seem to be repetitive code/execution. Can we return the applicable promotion as well in the findLowestPrice(product)
method?
@@ -116,6 +91,28 @@ export const getPriceData = (product, opts = {}) => { | |||
} | |||
} | |||
|
|||
export const findLowestPrice = (product) => { |
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.
Can you please add some jsx comment and unit tests for this function?
@@ -82,7 +83,7 @@ const ProductTile = (props) => { | |||
//TODO variants needs to be filter according to selectedAttribute value | |||
const variants = product?.variants | |||
|
|||
const priceData = getPriceData({...product, variants}) | |||
const priceData = useMemo(() => getPriceData({...product, variants}), [product, variants]) |
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 - Do we need variants as a dependency along with product to memorize as variants is a property with in product ?
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 variants
is there because the TODO comment made me to anticipate variants
to be updated in a later PR. And I don't know what it will look like later.
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.
@vmarta Can you try to update your branch with the feature branch? I believe we have some new code merged in that may look different from that you have on your current branch for the variant TODO above
@@ -168,6 +172,12 @@ const ProductTile = (props) => { | |||
return labelsMap | |||
}, [product, badgeDetails]) | |||
|
|||
const shouldShowPromoCallout = (product) => { |
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 move this function outside of the Component to avoid it being re-defined whenever the tile is re-render?
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. We can make it as a helper function (hasPromotions(product)
) in the product utils file
@@ -370,3 +380,9 @@ ProductTile.propTypes = { | |||
} | |||
|
|||
export default ProductTile | |||
|
|||
const shouldShowPromoCallout = (productWithFilteredVariants) => { |
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.
minor: make it a helper function hasPromotions(product)
in product-utils?
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 want to expose this particular function more globally. For example, due to the inconsistent API (productSearch vs getProduct), I should not be calling it on the Product Details page. So I'd rather constrain this function within ProductTile only.
.filter(Boolean) | ||
.filter(Number) | ||
return vals.length ? Math.min(...vals) : undefined | ||
const filtered = arr.filter((item) => Boolean(item[key])).filter((item) => Number(item[key])) |
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.
Nit: shouldn't filter by Number() will rule out falsy value too? 🤔
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.
In this case, I was following what's been done before: first filter with Boolean and then by Number.
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 gotcha, okay confirming that we don't need to filter by Boolean here since filter Number will already rule out falsy values. Checked in browser console.
const test = [null, undefined, 1, 2, 5]
test.filter(Number)
=> [1, 2, 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.
@alexvuong thanks for confirming it. There's something else, however. It was possible to ignore 0 in the calculation.
Now in the latest commit, if 0 is one of the values, it will be included.
* @param key | ||
* @returns {Array} an array of such smallest value and the item containing this value | ||
* @example | ||
* const [value, itemContainingValue] = getSmallestValByProperty(array, key) | ||
*/ | ||
const getSmallestValByProperty = (arr, key) => { |
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.
Nice touch on this util 👍
// a hit from productSearch call | ||
rollSleeveBlouse: { | ||
currency: 'GBP', | ||
hitType: 'master', |
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.
Do you want to remove any data that is not necessary for the promo call out test to reduce the file size? or you can reuse any data from app/mocks/product-search-hit-data.js?
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.
Nah, we don't need to worry about the weight of the test files :) They will not be included in the bundle, right?
I didn't want to worry about consolidating the mocks too. What if I have a particular product that does not exist in app/mocks/product-search-hit-data.js ?
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.
Minor comments, but LGTM overall
This PR implements the default behaviour for promotion callout. It relies on Alex's display-price work in her PR #1760 .
If the rendered price is a promotional price, we'll also render that promo's callout message in the ProductTile. Otherwise, if the variant has promotions, we'll show the callout from the very first promotion.
Types of Changes
Changes
How to Test-Drive This PR
Also, try other kinds of products:
Also, try other pages:
Checklists
General
Accessibility Compliance
You must check off all items in one of the follow two lists:
or...
Localization