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

Draft PR - Example for Supporting Missing Prices in Stencil Context Data #2484

Closed
wants to merge 1 commit into from

Conversation

bc-jz
Copy link
Contributor

@bc-jz bc-jz commented Sep 17, 2024

What?

This is an example of some template adjustments that will allow the PDP to support new pricing behavior where price can be excluded from variant data if not defined in an "Inclusive" or "Exclusive" price list. These changes specifically address issues brought up in BCTHEME-1941, namely:

  • If PDP page loads without a price then it breaks the HTML of the page and a price will never be shown even when selecting variants that have a price set
  • on PDP when selecting a variant that does not have a price set, we do not hide the price on the page but instead keep showing the previous selections price

Requirements

  • CHANGELOG.md entry added (required for code changes only)

Tickets / Documentation

Add links to any relevant tickets and documentation.

Screenshots (if appropriate)

Attach images or add image links here.

Example Image

@bc-yevhenii-buliuk
Copy link
Contributor

@bc-jz thanks for the suggested implementation, it looks good. Also, we need to consider the case when none of the product options are selected by default. Do you think we can similarly add an additional price--withoutTax / price--withTax classes for price-range.html that you use for price.html?

draft_PR_1941_case_3.mov

@bc-jz
Copy link
Contributor Author

bc-jz commented Sep 19, 2024

@bc-jz thanks for the suggested implementation, it looks good. Also, we need to consider the case when none of the product options are selected by default. Do you think we can similarly add an additional price--withoutTax / price--withTax classes for price-range.html that you use for price.html?

@bc-yevhenii-buliuk
Yes, I think the adjustments we make to price-range.html will be comparable. We mainly just need to make sure that the div that wraps around the "price now" section is given the same price--withTax or price--withoutTax class that was added into price.html, for example:
https://github.com/bigcommerce/cornerstone/pull/2484/files#diff-377f95bae5a6b0973d0ab2f44655a0bae14e006e651ee3c48e96b1e00f73d59aR32

We probably don't need to worry about adding the logic for display: none since if there is a price range provided it should always have a price. The output of price-range.html just needs to be comparable to price.html and we should validate they both play appropriately with the changes to the JS.


Side note, what do you think about removing the price--withTax and price--withoutTax classes from their existing placement on the span elements wrapping the price to reduce the chance to accidentally target them? For example here:

<span data-product-price-with-tax class="price price--withTax">{{price.with_tax.formatted}}</span>

I wrote my JS to specifically target the div elements and was not sure if those classes on the span are used anywhere else so left them in place but maybe that could be cleaned up.

@bc-yevhenii-buliuk
Copy link
Contributor

Side note, what do you think about removing the price--withTax and price--withoutTax classes from their existing placement on the span elements wrapping the price to reduce the chance to accidentally target them?

@bc-jz I agree with you and I also paid attention to this and I didn't see anywhere in the styles or JS logic where the price--withoutTax / price--withTax classes are used for the span element, so I think we can remove these unused classes. As a result, we won't need to specify this in the getViewModel method on the div element.

Indeed, at the first rendering of the price range, the price always exists and is taken only from the stencil object without an additional AJAX request for price clarification. Therefore, it is probably worth adding the price--withoutTax / price--withTax classes to the corresponding div elements to display/hide the price, just like in price.html. At the same time, we can also remove unused classes from span in price-range.html.

@bc-jz
Copy link
Contributor Author

bc-jz commented Sep 27, 2024

Superseded by #2486

@bc-jz bc-jz closed this Sep 27, 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