Skip to content

fix(storefront): BCTHEME-709 "Out of Stock" banner is duplicated and overlaps "Add to cart" button on PDP#2198

Closed
bc-vlad-dlogush wants to merge 1 commit intobigcommerce:masterfrom
bc-vlad-dlogush:BCTHEME-709
Closed

fix(storefront): BCTHEME-709 "Out of Stock" banner is duplicated and overlaps "Add to cart" button on PDP#2198
bc-vlad-dlogush wants to merge 1 commit intobigcommerce:masterfrom
bc-vlad-dlogush:BCTHEME-709

Conversation

@bc-vlad-dlogush
Copy link
Contributor

@bc-vlad-dlogush bc-vlad-dlogush commented Apr 11, 2022

What?

This PR removes the duplicate "Out of Stock" banner.

Tickets / Documentation

Screenshots (if appropriate)

before 709

after  709

before  AMP  709

after  AMP  709

Copy link
Contributor

@bc-jz bc-jz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not think this is a proper way to address this issue of the double banner being displayed as it does not work in all cases. Specifically if you have a complex product that does not have any default option values set, see below on my test store where I copied your changes in product-view.html:
Screen Shot 2022-05-20 at 9 47 44 AM
The add to cart button is gone but the out of stock message is not displayed. If I were to set at least one default option value then the message would display when loading this page.

Which brings me to my second point. The behavior to display the out of stock message with this code block removed is dependent on an ajax call to the remote product-attributes endpoint. When that call is made, if the response indicates that the product is out of stock then we will display the out of stock message. I do not think we should rely on that ajax call for displaying out of stock on an initial page load and really should try to eliminate use of that call altogether.

I will add some more details shortly about where that call is being made and how I think we should address this issue of the double error messages.

@bc-jz
Copy link
Contributor

bc-jz commented May 20, 2022

This logic here is what is triggering on initial page load whenever there are default option values set. It fires the remote product-attributes ajax call and is using the results to update the option data on the page including the OOS state of the product:
https://github.com/bigcommerce/cornerstone/blob/master/assets/js/theme/common/product-details.js#L90-L94

This code is what is adding the 2nd error message when the selected option values are OOS:
https://github.com/bigcommerce/cornerstone/blob/master/assets/js/theme/common/product-details-base.js#L394-L397

Traditionally this was needed because the context that we provided to the page was incomplete and incorrect for certain situations:

  • partial default option selections would not indicate OOS status or price correctly
  • products tracking inventory by variant did not indicate OOS state
  • the "in_stock_attributes" data provided on initial page load could be wrong in certain situations

I believe we are at a point now with the stencil context where these items have been corrected and we can move away from making this ajax call on initial page load altogether. The data received in the stencil context should correctly match up with any default selections if there are any and take partial selections into account. The last piece of this was merged yesterday with this fix to OOS labeling when products are tracking inventory by variant:
https://github.com/bigcommerce/bigcommerce/pull/46317

So rather than remove the checks to {{product.out_of_stock}} that is being done in this PR we should leave those and instead remove this ajax call that can fire on initial page load.

@bc-jz
Copy link
Contributor

bc-jz commented May 20, 2022

In summation, the reasons I recommend going this more complex route for a fix are:

  • we should be able to load the initial state of the page purely from stencil context which is more performant than needing follow up calls
  • we should strive to eliminate making additional bcapp calls wherever possible for the overall health of our platform

@jordanarldt
Copy link
Contributor

Hey @bc-vlad-dlogush I had some free time so I made a draft PR to fix the issue while also addressing Jason's points. Feel free to take a look here:
https://github.com/jordanarldt/cornerstone/pull/3/files

@bc-as
Copy link
Contributor

bc-as commented Aug 23, 2022

@BC-krasnoshapka @bc-vlad-dlogush can you check out @jordanarldt 's solution to see if this hits all of the points that @bc-jz mentioned?

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.

8 participants