Skip to content
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

Open
wants to merge 6 commits into
base: develop
Choose a base branch
from

Conversation

htuzel
Copy link
Collaborator

@htuzel htuzel commented Dec 5, 2024

What is changed

  • Added support for pricebooks for both variant and product level records.
  • Fixed some bugs about displaying prices wrongly by testing different cases for variant and product level records.
  • Moved some calculations to calculateDisplayPrice make things more clear and using common logic for different records models

@htuzel htuzel requested a review from sbellone December 5, 2024 13:05
Copy link
Collaborator

@sbellone sbellone left a 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

@@ -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) => {
Copy link
Collaborator

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`
Copy link
Collaborator

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

@htuzel htuzel self-assigned this Dec 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants