-
Notifications
You must be signed in to change notification settings - Fork 4
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: added pricebook support for variants and product level records #208
base: develop
Are you sure you want to change the base?
Conversation
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.
Pricebooks logic has been broken 😬
Fixed some bugs about displaying prices wrongly by testing different cases for variant and product level records.
Can you give details so I can test? To be sure that it didn't break something else
cartridges/int_algolia_sfra/cartridge/static/default/js/algolia/instantsearch-config.js
Show resolved
Hide resolved
@@ -457,6 +439,10 @@ function enableInstantSearch(config) { | |||
|
|||
item.displayPrice = variantPrice; | |||
item.calloutMsg = variantCalloutMsg; | |||
|
|||
item.highestPriceBookPrice = selectedVariant.pricebooks && selectedVariant.pricebooks[algoliaData.currencyCode] && selectedVariant.pricebooks[algoliaData.currencyCode].length > 0 ? selectedVariant.pricebooks[algoliaData.currencyCode].reduce((max, pricebook) => { |
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 doesn't makes things more clear: a 262 characters line with a ternary check at the end that triggers a function is quite hard to process.
You should break it down.
@@ -287,9 +287,9 @@ function enableInstantSearch(config) { | |||
<div class="price"> | |||
${isPricingLazyLoad && html`<span class="price-placeholder" data-product-id="${productId}"></span>`} | |||
${!isPricingLazyLoad && html` | |||
${ (hit.displayPrice < hit.price || (hit.promotionalPrice && hit.promotionalPrice < hit.price)) && html` | |||
${ (hit.displayPrice < hit.price || (hit.promotionalPrice && hit.promotionalPrice < hit.price) || (hit.highestPriceBookPrice && hit.highestPriceBookPrice > hit.displayPrice)) && html` |
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.
You've made things more clear with the calculateDisplayPrice
function, but you have complexified this part.
It's a good occasion to improve the transformItem
function and generate prices ready to be displayed:
- a
strikeout
price if there is one - a
displayPrice
So the HTML doesn't need such complex logic
What is changed
calculateDisplayPrice
make things more clear and using common logic for different records models