-
Notifications
You must be signed in to change notification settings - Fork 10.8k
Render cart items in Mini Cart drawer with iAPI, support increase / decrease quantity #58370
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
Conversation
…tion of experimental rendering.
…r close button for drawer.
…mmerce, woocommerce/client/admin
…n interim workaround strip it out.
… mini cart title up to date.
…tils to format prices in MiniCartFooterBlock
… into dev/refactor-price-utils
Size Change: +384 B (+0.01%) Total Size: 5.97 MB
|
Test using WordPress PlaygroundThe changes in this pull request can be previewed and tested using a WordPress Playground instance. Test this pull request with WordPress Playground. Note that this URL is valid for 30 days from when this comment was last updated. You can update it by closing/reopening the PR or pushing a new commit. |
Testing GuidelinesHi @luisherranz , Apart from reviewing the code changes, please make sure to review the testing instructions (Guide) and verify that relevant tests (E2E, Unit, Integration, etc.) have been added or updated as needed. Reminder: PR reviewers are required to document testing performed. This includes:
|
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, Sam. This is looking great so far 🙂👏
Pre-approving with some comments.
$reduce_quantity_label = __( 'Reduce quantity of %s', 'woocommerce' ); | ||
|
||
// translators: %s is the name of the product in cart. | ||
$increase_quantity_label = __( 'Increase quantity of %s', 'woocommerce' ); | ||
|
||
// translators: %s is the name of the product in cart. | ||
$quantity_description_label = __( 'Quantity of %s in your cart', 'woocommerce' ); | ||
|
||
// translators: %s is the name of the product in cart. | ||
$remove_from_cart_label = __( 'Remove %s from cart', 'woocommerce' ); |
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.
In the Product Button block, Luigi replaced %s
with ###
. I guess that's not necessary, right? We should probably change it there too, for consistency.
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.
I don't see why it would be necessary. I can ask him about it and do a follow up for consistency.
plugins/woocommerce/client/blocks/assets/js/blocks/mini-cart/iapi-frontend.ts
Outdated
Show resolved
Hide resolved
plugins/woocommerce/client/blocks/assets/js/blocks/mini-cart/iapi-frontend.ts
Outdated
Show resolved
Hide resolved
}, | ||
|
||
// Intended to be used in context of a cart item in wp-each | ||
itemShortDescription() { |
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.
My take is that this should be moved to callbacks
. I understand that it is due to the limitation of not yet having a directive for the HTML, but I'd move it anyway to keep only state and derived state inside state
.
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.
Yeah that makes sense 👍🏻
</div> | ||
<div class="wc-block-components-product-metadata"> | ||
<div data-wp-watch="state.itemShortDescription" > | ||
<div class="wc-block-components-product-metadata__description" data-wp-ignore></div> |
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.
Oh, this is very interesting. I wonder why you need data-wp-ignore
here. I was thinking about deprecating and the removing it, as we haven't found any use case for it (and it's a very complex and confusing directive).
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.
Ah I forgot to revisit this 🤦🏻♂️ . I assumed by the name of it that it was a way to tell interactivity API that I don't want it to control rendering this region, now looking at it I see it is sorta like that, but would likely only make sense to do it, if it has some initial inner content to cache? I think I can remove this.
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.
I think it's okay to delete it and see if everything works as it should. If it doesn't work, we'll research why.
plugins/woocommerce/client/blocks/assets/js/blocks/mini-cart/iapi-frontend.ts
Outdated
Show resolved
Hide resolved
@luisherranz Could you help me debug tomorrow, there seem to be some issues with reflecting state updates, I can't work out where it's going wrong. I'm wondering if server side directive handling is part of the issue? |
@luisherranz just adding some more thoughts: A revert to the way we were using context before works: 3ff8785 We should still figure out whats going on, but one thing I don't like about the changes you made is its quite hard to reason about. I come back to finding the "context" being set to It results in less code, but overall a lot more confusing in my opinion. Still would be great to pair and try figure out why the current pattern is not working properly for me. EDIT: I kept going with a new branch for the follow on quantity limits work. This branch: https://github.com/woocommerce/woocommerce/compare/dev/redo-cart-set-qty?expand=1 removes the duplicated |
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.
After investigating with @samueljseay and @DAreRodz, we discovered that the problem is a bug in data-wp-each
, and @DAreRodz will prepare a pull request in the Interactivity API to solve it.
There is still an issue in the lineItemTotal
update, for which we have to find a workaround.
@luisherranz how do you feel about merging this so we can continue? I've continued to iterate in #58589 and also in https://github.com/woocommerce/woocommerce/compare/wooplug-4502-interactivity-api-powered-mini-cart-display-salediscount?expand=1 |
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.
Yes, let's do that. Sorry for the request changes.
…ecrease quantity (#58370) Co-authored-by: Luis Herranz <luisherranz@gmail.com>
Changes proposed in this Pull Request:
This PR introduces rendering of the mini cart items in the drawer via the interactivity API (behind experimental flag like in previous PRs).
Not all functionality is yet supported but increasing/decreasing quantities is supported via the UI steppers.
Also note that there is a flicker of items when changing quantities. I plan to address improvements to the store and this issue in a follow up PR. I will also support removing items entirely in a separate PR
Screenshots or screen recordings:
Screen.Recording.2025-06-02.at.2.26.51.PM.mov
How to test the changes in this Pull Request:
+
and-
steppers to increase/decrease quantities of the items (noting that removing/going to 0 is not implemented yet)Testing that has already taken place:
Changelog entry
Changelog Entry Details
Significance
Type
Message
Changelog Entry Comment
Comment
[Experimental] Render mini cart items using interactivity API when feature flag is enabled.