Product button: sync temporary number of items on instantiation #10604
Conversation
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
assets/js/interactivity/directives.js
assets/js/interactivity/hooks.js |
|
Size Change: +76 B (0%) Total Size: 1.43 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Hey @luisherranz,
Appreciate the prompt resolution of the issue. I've tested the modifications, and it seems the problem is resolved 🚀.
Additionally, the code changes look good to me. As I'm still acquainting myself with the intricacies of coding related to the Interactivity API, it would be helpful if either @DAreRodz or @gigitux could review the code as well.
Thanks! 🙌🏻
I guess this is ok to merge because it fixes the pagination issue, but we need to make Screen.Capture.on.2023-08-16.at.10-21-45.mp4 |
|
By the way, I just checked and it seems to work fine with the Products (beta) block as well because the Products Template block is the same, but it'd be great if someone could confirm this 🙂 |
I tested with the Products beta block and it doesn't work: Screen.Capture.on.2023-08-16.at.11-16-12.mp4On the Since we will migrate all the Products (beta) blocks to the Product Collection and deprecate the Products (beta) block, I would enable #10200 only for the Product Collection. Thoughts? cc @kmanijak @imanish003 |
|
Ok. Thanks for checking this out, @gigitux 🙂 |
|
Hey @gigitux,
If it's not feasible to include @luisherranz FYI If it’s not possible to add
|
I'm not saying that you should, but in theory you could add it using the WP_HTML_Tag_Processor and the Anyway, I think it'd be simpler to add it to the new block only if the old one is going to be deprecated soon.
By the way, we still need to figure out the (auto) opt-in/out mechanism. |
May I know what you mean by |
Yeah, we can follow this approach, but Product Collection will be released soon so I don't see any advantages to do this kind of work.
Can you expand this? |
|
The client-side router knows how to hydrate interactive blocks that use the Interactivity API (like the Product Button block), but it doesn't know how to hydrate interactive blocks that rely on DOM ready events or other methods for hydration. Therefore, if people add external interactive blocks to the Product Template, they won't work after the first client-side navigation. We need to figure out an opt-in or an opt-out mechanism to enable/disable the client-side navigation. It can be manual or we may try to do something that is more automatic. Either way, the site owner should be able to overwrite its value, so probably the options should be We could start with an |
There was a problem hiding this comment.
I see it great codewise. 😄 ✅
BTW, @luisherranz, what do you think of moving the layout-init logic to the regular init directive in the future? E.g., init could also accept an object, which would contain the callback path and if it's going to do a layout change.
<button data-wc-init='{ "path": "init.ItemsOnLoad", "layout": true }'>I'm aware that this would make directives bigger; I'm just curious about your opinion on this.
|
@gigitux @imanish003, regarding the (auto) opt-in/out mechanism and the |
It's an interesting alternative. Would you mind sharing it in this discussion? |
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
|
I think that we can merge this PR, right? cc @imanish003 @luisherranz |
github-actions
bot
removed
the
status: stale
I think so. I plan to review it once more today to ensure that everything is functioning as intended. |
There was a problem hiding this comment.
I've tested the fix, and I can confirm that everything is working as expected. I greatly appreciate the video explanation, @luisherranz. It was incredibly helpful in comprehending the modifications made in this pull request. 🚀
|
This PR has been marked as If deemed still relevant, the pr can be kept active by ensuring it's up to date with the main branch and removing the stale label. |
github-actions
bot
added
the
status: stale
It's fine to me. We will improve it in the future. |
@luisherranz I believe we're ready to proceed with merging this PR. If you're in agreement, could you please merge it? |
|
Oh, I didn't realize it had to be me the one merging it. I'll merge it now. |
tarhi-saad
added
type: bug
What
I've fixed a sync problem between the number of items in the cart (
state.woocommerce.cart) and the number of items that we show in the Product Button (context.woocommerce.temporaryNumberOfItems).Why
We use two values to be able to trigger an animation when those values are different (for example, when updated from the MiniCart). When using client-side navigation, we want to show the number in
state.woocommerce.cart, not incontext.woocommerce.temporaryNumberOfItemsbecause the cart is the source of truth. Also, we don't want to trigger an animation, we want to show it right away.How
I've used a
useLayoutEffectto sync thetemporaryNumberOfItemswith the cart items when the element is instantiated and the cart (state.woocommerce.cart) has already loaded. I don't love it because it means manually syncing both states, which is something we should try to avoid whenever possible.Anyway, I'll think more about this pattern, but this should fix it for now.
It also adds
key's to the Product Collection array.Screenshots
https://www.loom.com/share/c43275ccb80d4286b96da2c3b75e9280
Testing
WooCommerce Visibility
Checklist
Required:
[type]label or a[skip-changelog]label.Conditional:
[skip-changelog]label is not present).Changelog