Skip to content
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.

Sign up for GitHub

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

Categories Block: Add iAPI directive for client-side routing #64907

Merged
merged 3 commits into from
Sep 4, 2024
Merged

Categories Block: Add iAPI directive for client-side routing #64907

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f2afb10
Categories Block: Add iAPI directive for client-side routing
ockham Aug 29, 2024
d9dc1b7
Respect enhancedPagination context
ockham Aug 29, 2024
766f888
WPCS
ockham Sep 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions packages/block-library/src/categories/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,12 @@ function render_block_core_categories( $attributes ) {
$wrapper_markup = '<ul %1$s>%2$s</ul>';
$items_markup = wp_list_categories( $args );
$type = 'list';

$p = new WP_HTML_Tag_Processor( $items_markup );
Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't it be the responsibility of the Query Loop block to intercept all clicks to links instead when the proper setting is enabled? Otherwise, the same change would have to be applied to every possible block that contains link.

Copy link
Contributor

Choose a reason for hiding this comment

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

Indeed. We should add the data-wp-on-click directives in the Query block's render callback.

However, in order to find where to add them, we should also add some "markers" here (in the Categories block).

One solution could be something like adding data-wp-navigation-link attributes (which were removed in #57853):

// Categories block
while ( $p->next_tag( 'a' ) ) {
	$p->set_attribute( 'data-wp-navigation-link', );
}
// Query block
while ( $p->next_tag( 'a' ) ) {
	if ( $p->get_attribute('data-wp-navigation-link') ) {
	  $p->set_attribute( 'data-wp-on--click', 'core/query::actions.navigate' );
	}
}
$items_markup = $p->get_updated_html();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, folks, that's exactly the kind of feedback I was looking for!

@gziolo wrote:

Wouldn't it be the responsibility of the Query Loop block to intercept all clicks to links instead when the proper setting is enabled? Otherwise, the same change would have to be applied to every possible block that contains link.

@michalczaplinski wrote:

Indeed. We should add the data-wp-on-click directives in the Query Loop block's render callback.
However, in order to find where to add them, we should also add some "markers" here (in the Categories block).

I like the idea of adding the directives from within the Query block's render callback, as that gives us access to the information whether enhanced pagination is on or off.

However, we need to decide on an overall strategy:

  • Doing it wholesale for all links found inside of the Query Loop block, as @gziolo suggested, or
  • Applying it only to individual blocks that have opted in, e.g. via some sort of marker (another data-wp- attribute), as suggested by @michalczaplinski.

I think both approaches have their tradeoffs:

  • Doing it wholesale might erroneously apply to links that point other parts of the site (that use a different template and would thus break region-based client-side navigation), or even external links. OTOH.
  • Requiring opt-in adds some overhead. Furthermore, in terms of how blocks opt into this behavior, I don't really love the idea of using another data-wp attribute just to tell WP to replace it with data-wp-on--click='core/query::actions.navigate'. (To me, it raises the question what we're gaining from that, vs. having the block set the data-wp-on--click attribute itself.)

We might be able to tweak both strategies a bit:

  • Wholesale: We could make sure to only add the data-wp-on--click handler to links that point to routes that are handled by the same template. (This could be a bit tricky in practice due to WP's template hierarchy; the system would need to know a lot about routes and templates.)
  • Opt-in: Instead of setting a marker on each instance of a given block; can we just opt the entire block type in, i.e. set a block attribute or something of the sort?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see that the Query Loop block provides enhancedPagination context, which is used by the Post Template block. This looks promising to me, as it might allow us to minimize the extra information we introduce and/or communicate. It would still leave the responsibility of setting data-wp-on--click='core/query::actions.navigate' attribute with individual blocks, but based on the above, that might be reasonable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(Note that not all blocks will require the HTML API to inject the attribute; we need it for the core/categories block as that block gets its HTML from wp_list_categories() and wp_dropdown_categories, respectively. That HTML is opaque to us, so we have to resort to the HTML API in order to modify it; but in other cases, we'll be able to simply include the data-wp-on--click attribute in the HTML string we return from the block.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried it in d9dc1b7. I kinda like this 😬

Copy link
Member

Choose a reason for hiding this comment

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

I don't have much to add here.

  • It's true that click handlers cannot be added to all the clicks within the Query loop because there are links that may point to other parts of the site.
  • I don't have a strong opinion on how to include those click handlers, but in my opinion, the way we are currently doing it, passing whether the enhanced pagination is active through block context and injecting the click handlers into each block, seems simple and appropriate for now.
  • Sometimes, there will be more complex cases where a click-handler is not enough, and more logic needs to be added, such as with the Search block, so it's important to leave room for custom implementations.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much for weighing in, @luisherranz!

I'll go ahead and open this for review then 😊

Copy link
Member

Choose a reason for hiding this comment

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

I left more thoughts related to that topic #64907 (comment) in response to the #64907 (review) from @dmsnell.

while ( $p->next_tag( 'a' ) ) {
$p->set_attribute( 'data-wp-on--click', 'core/query::actions.navigate' );
}
$items_markup = $p->get_updated_html();
}

$wrapper_attributes = get_block_wrapper_attributes( array( 'class' => "wp-block-categories-{$type}" ) );
Expand Down
Loading