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
Fix display of "empty cart" notices when using a shortcode cart and the checkout block #38738
Conversation
Hi @jorgeatorres, @nielslange, Apart from reviewing the code changes, please make sure to review the testing instructions as well. You can follow this guide to find out what good testing instructions should look like: |
Test Results SummaryCommit SHA: 6592168
To view the full API test report, click here. To view the full E2E test report, click here. To view all test reports, visit the WooCommerce Test Reports Dashboard. |
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.
Thanks for working on this, @mikejolley.
While reviewing the PR, I wasn't able to see the 2nd screenshot that you mentioned.
Cart shortcode:
|
Checkout shortcode:
|
Cart block:
|
Checkout block:
|
Do you have any idea on why the design of your 2nd screenshot is not visible on my end?
Apart from the screenshot, I found the 2nd testing instructions ("Next, edit your CHECKOUT page ...") a bit confusing. While the title says to replace the checkout shortcode with the Checkout block, the instructions do not mention tests on the checkout page. To me, it was not clear whether I should check the checkout page or not.
@nielslange you'll only see the new design if the checkout page assigned under WooCommerce > settings > advanced actually contains the checkout block, hence the instructions "Next, edit your CHECKOUT page and insert the checkout block in place of the shortcode." |
Hey @mikejolley! There are a couple of PHPCS violations (see https://github.com/woocommerce/woocommerce/actions/runs/5277832291/jobs/9546360759?pr=38738) which need to be addressed before merging this. I can confirm that the PR does fix the "empty cart" notice situation (when the checkout page uses the checkout block). Even if not directly related to this PR, I noticed a couple of things while testing, which maybe you can comment on:
FWIW, I'm testing with StoreFront. |
I did that, @mikejolley, but I'll double-check it. It appears that there's something wrong with my local environment the previous days. Somehow, I keep getting different results than others. |
@mikejolley I tested it again, and I'm getting the same result. When using the Cart block, I do not see the notice. Here are the steps I took:
This only happens when using the Cart block. When using the Cart shortcode, I do see the notices. |
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.
Thanks for working on this, @mikejolley.
I've tested this PR successfully with the following setup and result:
Test case 1 | Test case 2 | |
Page Setup » Cart page |
|
|
Page Setup » Checkout page |
|
📦 Checkout block |
Screenshot » Cart page |
Thanks @nielslange @jorgeatorres I fixed those up. Notice styling is expected to differ due to blocks changing the notice markup and including its own styles. |
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.
LGTM
Submission Review Guidelines:
Changes proposed in this Pull Request:
C&C Blocks introduced some new notice markup via some updated templates. These templates use a different HTML structure than the notices in core. The core notices get a special classname added in
wc_empty_cart_message
calledcart-empty
, but this is not added to the new Block notices because of the updated markup. For reference:Cart.js uses the
cart-empty
to target elements from ajax responses. This is handled withinupdate_wc_div
. Because thecart-empty
class does not exist when using blocks, this update fails and so after interacting with the shortcode cart, the page is emptied.The fix in this PR is to wrap everything returned by
wc_empty_cart_message
with a newwc-empty-cart-message
classname, which is then used by cart.js so the update works in both contexts.Closes #38719
How to test the changes in this Pull Request:
This needs to be tested twice to ensure there are no regressions.
Test 1:
For the pages assigned as cart/checkout under WooCommerce > Settings > Advanced:
Test 2
For the pages assigned as cart/checkout under WooCommerce > Settings > Advanced:
Notice the styling is updated between tests.