bigcommerce / cornerstone

The BigCommerce Cornerstone theme
https://developer.bigcommerce.com/stencil-docs
286 stars 606 forks source link

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

Closed bc-jz closed 1 week ago

bc-jz commented 2 weeks ago

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:

Requirements

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 commented 2 weeks ago

@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?

https://github.com/user-attachments/assets/d101cb4a-684d-4cb7-8b04-473897ae9bd9

bc-jz commented 2 weeks ago

@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: https://github.com/bigcommerce/cornerstone/blob/98d2e4032607621303c2b385ac494cc16b7aa13d/templates/components/products/price.html#L42

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 commented 2 weeks ago

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 commented 1 week ago

Superseded by https://github.com/bigcommerce/cornerstone/pull/2486