Interactivity API: Update interactive regions during client-side navigation #10200
Conversation
|
The release ZIP for this PR is accessible via: Script Dependencies ReportThe
This comment was automatically generated by the TypeScript Errors Report
🎉 🎉 This PR does not introduce new TS errors. |
| if ( | ||
| self::is_woocommerce_variation( $this->parsed_block ) && | ||
| $instance->context['queryId'] === $this->parsed_block['attrs']['queryId'] | ||
| ) { |
There was a problem hiding this comment.
I used this check to ensure that we're adding the navigation-link directive only to the links inside the Pagination Block contained in the Products Query block. If you think there's a better way to do this, feel free to update the code. 😊
There was a problem hiding this comment.
Yeah, it'd be great if someone from Woo could review this part and give a thumbs up 🙂
There was a problem hiding this comment.
I would add another check:
core/query' === $block['blockName'] && self::is_woocommerce_variation( $this->parsed_block )....The rest LGTM!
|
Size Change: +950 B (0%) Total Size: 1.36 MB
ℹ️ View Unchanged
|
| ) { | ||
| $p = new \WP_HTML_Tag_Processor( $block_content ); | ||
|
|
||
| while ( $p->next_tag( 'a' ) ) { |
There was a problem hiding this comment.
There was a problem hiding this comment.
Good question. I don't have a strong opinion, but prefetching the next and previous ones and skip the others makes sense to me.
There was a problem hiding this comment.
I'll make that change.
| if ( | ||
| self::is_woocommerce_variation( $this->parsed_block ) && | ||
| $instance->context['queryId'] === $this->parsed_block['attrs']['queryId'] | ||
| ) { |
There was a problem hiding this comment.
Yeah, it'd be great if someone from Woo could review this part and give a thumbs up 🙂
There was a problem hiding this comment.
I did a brief review and overall it looks good to me. Good work and exciting stuff! 👏 I will let the others do the proper review as they have more context on the Interactivity API.
One question I have is whether the plan is to merge this so it works with the Products (Beta) block or whether it would be better to wait until the Product Collection block is released. Has this been discussed? For context: the Product Collection block is a replacement for the Products (Beta) block that won't use the Gutenberg Query Loop block underneath.
Besides that, I left a couple of minor comments below. 👇
| ) { | ||
| $p = new \WP_HTML_Tag_Processor( $block_content ); | ||
|
|
||
| while ( $p->next_tag( 'a' ) ) { |
There was a problem hiding this comment.
Good question. I don't have a strong opinion, but prefetching the next and previous ones and skip the others makes sense to me.
src/BlockTypes/ProductQuery.php
Outdated
| while ( $p->next_tag( 'a' ) ) { | ||
| $p->set_attribute( | ||
| 'data-wc-navigation-link', | ||
| '{"prefetch":true,"scroll":false}' |
There was a problem hiding this comment.
I guess it depends on the use-case, but when using the Products block in the Product Catalog template, I think it should default to scrolling to top.
There was a problem hiding this comment.
I'll make this change as well. 🙂
|
I've addressed most of your suggestions. Ready for review again. 😊 |
There was a problem hiding this comment.
This looks great, David. Amazing work 👏
Let's wait for a review from Woo, tho. Especially this bit and an answer to these Albert's questions.
| if ( $is_previous ) { | ||
| $p->set_attribute( 'key', 'pagination-previous' ); | ||
| } elseif ( $is_next ) { | ||
| $p->set_attribute( 'key', 'pagination-next' ); | ||
| } |
There was a problem hiding this comment.
Cool. Time to add support for data-wc-key to avoid adding invalid HTML?
There was a problem hiding this comment.
I just added a comment but it isn't a critical change. So LGTM! Nice work!
I want to echo the @Aljullu comment:
One question I have is whether the plan is to merge this so it works with the Products (Beta) block or whether it would be better to wait until the Product Collection block is released. Has this been discussed? For context: the Product Collection block is a replacement for the Products (Beta) block that won't use the Gutenberg Query Loop block underneath.
I don't think that it is a critical issue porting these changes on the Product Collection block, btw.
cc @imanish003 @kmanijak (they are working on the Product Collection)
| * @param \WP_Block $instance The block instance. | ||
| */ | ||
| public function add_navigation_id_directive( $block_content, $block, $instance ) { | ||
| if ( self::is_woocommerce_variation( $block ) ) { |
There was a problem hiding this comment.
I would add another check:
core/query' === $block['blockName'] && self::is_woocommerce_variation( $this->parsed_block )The rest LGTM!
@gigitux, thanks for the ping! First of all, I love the UX improvements this PR brings! And having said that, I think it is crucial to port these to Product Collection as soon as possible. When migrating from Products (Beta) to Product Collection, users would see a decreased performance, which could be perceived as a regression. I don't think it has to be covered within this PR, but would be great to cover it before or right after the initial release. We'd like to release Product Collection in one of the upcoming releases (1-2). BTW, I may have a wrong impression, but checking the changes in this PR, it doesn't seem too hard to re-use that in Product Collection (I mean especially for someone experienced with Interactivity API). |
|
I agree with what has been mentioned so far:
|
Huh, good catch! Actually this should be 6.2. We officially only support the latest WP version. 🤔
Once WP 6.3 is released in August, WC will update its supported versions to 6.2 or higher, so we should be good to go. So, if I got the dates right, the tl;dr is:
|
|
I think @Aljullu has summed it up nicely, especially if the following statement is true:
My assumption is that this behaviour will be most valuable to mobile users, but we like those have already said below me we shouldn't assume that the product grid is always going to extend beyond the height of the users screen.
I think we should also avoid doing this. |
It does, but it also scrolls to the bottom when the list is bigger than the screen. I've tried both
https://codepen.io/luisherranz/pen/ExOdjRK For now, my opinion would be to go with
Maybe we could try a new pattern for this: passing a callback. <a
href="..."
data-wc-navigation-link='{ "prefetch": true, "scroll": "ref:actions.woocommerce.scrollToProducts" }'
></a>store({
actions: {
woocommerce: {
scrollToProducts: ({ ref }) => {
ref.closest("[data-wc-navigation-id^='woo-products-'").scrollIntoView({
behavior: "smooth",
block: "start",
});
},
},
},
});In the future, if what Albert mentioned here is the desired UX, we could introduce an EDIT: I documented this pattern here. |
Oh, I wasn't aware of that. Thanks for pointing it out!
It works for me. 👍 We can always iterate on this in the future if needed. |
|
Hey @luisherranz,
Thank you for sharing this and documenting this approach. This looks promising to me. I noticed that you have also commented on draft PR #10349. I have a few questions regarding this approach, which I will ask on the PR 🙂 As far as I understand, it seems that what @Aljullu mentioned here is the ideal behavior we would like to achieve. If so, should I continue working on draft PR #10349 and see if this could be achieved using the approach shared by @luisherranz in the above comment? CC: @tjcafferkey |
|
I am going to bump the milestone of this PR as this doesn't have a blocker label. |
|
Hey, I'm just catching up with the conversation. I see a PR already opened (#10349) to explore the scroll behavior that was agreed upon (i.e., scroll smoothly to the top of the nearest Query block containing the link). In any case, I guess we can merge this PR as it is, right? Or should we wait for the other PR to be ready? 🤔 There is also this discussion to figure out a better API for directives that could enable these use cases. |
|
I would disable the scroll for now ( |
|
@luisherranz @DAreRodz Just to confirm, it looks like before merging this PR, we haven't disabled the scroll for now. That is intended, right? I am confirming this because I will be implementing the same for the Product Collection block too. |
|
Oops! 🙈 Nope, it wasn't intended; nice catch @imanish003. I think I assumed it was disabled when I merged it... 😅 I can open a PR with a quick fix later today. |
|
Sounds great! Thanks, @DAreRodz 🙌🏻 |
|
I almost forgot; here it is. |
nielslange
added
type: enhancement
With this PR, the Interactivity API exposes two directives to ease updating the Products Query block during client-side navigation.
data-wc-navigation-id: defines an ID for an interactive region (i.e., an element with thedata-wc-interactiveattribute). The ID matches the region between different pages and is used to render the SSRed content.data-wc-navigation-link: formerlydata-wc-link, just renamed to include thenavigationnamespace.All the logic related to the client-side navigation meta tag has been removed, as it is not needed from now on.
In addition, I've added a couple of filters to the Products Query block in order to inject the directives mentioned above. You can see it working in this video:
Screen.Recording.2023-07-14.at.10.14.10.mov
Screen.Recording.2023-07-14.at.10.04.08.mov
Tracking issue: woocommerce/woocommerce#42486
Testing
Automated Tests
User Facing Testing
perPageprop inside thequeryattribute to1./shop).WooCommerce Visibility
Performance Impact
Changelog