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.

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

Should we provide multi-query comments loops? #36642

Closed
cbravobernal opened this issue Nov 19, 2021 · 12 comments
Closed

Should we provide multi-query comments loops? #36642

cbravobernal opened this issue Nov 19, 2021 · 12 comments
Assignees
Labels
Needs Design Feedback Needs general design feedback. Needs Technical Feedback Needs testing from a developer perspective. [Type] Discussion For issues that are high-level and not yet ready to implement. [Type] Question Questions about the design or development of the editor.

Comments

@cbravobernal
Copy link
Contributor

cbravobernal commented Nov 19, 2021

Summary of the question

If we take a look at the Post Query Loop. It is ready so the user can have multiple queries on the same page.

useEffect( () => {
if ( ! queryId ) {
__unstableMarkNextChangeAsNotPersistent();
setAttributes( { queryId: instanceId } );
}
}, [ queryId, instanceId ] );

Then in PHP we read that queryId and get the info to be able to make different queries on the same page.

$page_key = isset( $block->context['queryId'] ) ? 'query-' . $block->context['queryId'] . '-page' : 'query-page';
$page = empty( $_GET[ $page_key ] ) ? 1 : (int) $_GET[ $page_key ];
$query_args = build_query_vars_from_query_block( $block, $page );
// Override the custom query with the global query if needed.
$use_global_query = ( isset( $block->context['query']['inherit'] ) && $block->context['query']['inherit'] );

This feature is completely fine for posts. For example, in a blog, you may want to create a couple of "Posts per category" blocks. But you can also add pagination to them. This pagination is working also individually, and, sometimes, can create weird results.

For example, if you create a query loop and set the layout as two columns, you can add a different pagination per column (maybe we have to fix that in another PR).

The question is:

Should we provide multi-query comments loops? Is it a nice feature to have different comment loops on the same page?

Depending on this discussion result, the code for the comment loop and pagination can be more or less complex.

@cbravobernal cbravobernal added [Type] Question Questions about the design or development of the editor. Needs Design Feedback Needs general design feedback. Needs Technical Feedback Needs testing from a developer perspective. [Type] Discussion For issues that are high-level and not yet ready to implement. labels Nov 19, 2021
@luisherranz
Copy link
Member

If I'm not mistaken, WordPress comment pagination is using this URL structure right now:

  • https://mysite.com/post/
  • https://mysite.com/post/comment-page-2/
  • https://mysite.com/post/comment-page-3/
  • ...

So, if we want to make this backward compatible with the existing URL structure, we need to use the /comment-page-x/ path.

I guess we have a previous example of this. The WordPress archives, which follow this URL structure:

  • https://mysite.com/category/something/
  • https://mysite.com/category/something/page/2/
  • https://mysite.com/category/something/page/3/
  • ...

I didn't look at the code, but I did a quick test with a block theme, and it seems that in archives, the Query Loop block is following that structure:

But if I add a Query Loop block to a page, it uses the queryId pagination:

So I guess it may be using the old WordPress URL structure when the query is inherited and the new queryId when it's not.

That would mean you can use two query loops in an archive, one inherited and one custom, and the URLs would be backward compatible 🙂

@c4rl0sbr4v0, would you mind taking a look at the code of the Query Loop and/or related issues to confirm that it works that way? If so, I think it would make sense to implement it that way.

@DAreRodz
Copy link
Contributor

If I'm not mistaken, WordPress comment pagination is using this URL structure right now:

Just a quick note. I think (not sure tho) that the /post/comment-page-1/ URL would also work, and that /post/ (the default one) would point to either the first page (/post/comment-page-1/) or the last one (/post/comment-page-n/), depending on the order configured in the comments settings.

Can anybody check that?

@cbravobernal
Copy link
Contributor Author

So, if we want to make this backward compatible with the existing URL structure...

It seems that it gets the comment page from a query variable inside the GET petition. So it doesn't mind the URL permalink you are using. It works the same with /comment-page-3/ or ?cpage=3.

would you mind taking a look at the code of the Query Loop

Sure, what they do is reset the query, so the main query does not interfere with the query loop. They use offset in order to set the first comment to show, instead of pagination variable:

// Unset `offset` because if is set, $wp_query overrides/ignores the paged parameter and breaks pagination.
unset( $query_args['offset'] );
$query_args = wp_parse_args( $wp_query->query_vars, $query_args );

And after that, it creates a new one with the arguments got from the block.

Query args:

$query_args = build_query_vars_from_query_block( $block, $page );

Query created:

$query = new WP_Query( $query_args );

After rendering the comment blocks, it resets in order to keep the first query working on the rest of the page/site:

For the query pagination links, what they do is to add a query argument into the HTML tag href=url_with_new_query_arg. This is coded here:

Create the page key:

$page_key = isset( $block->context['queryId'] ) ? 'query-' . $block->context['queryId'] . '-page' : 'query-page';

Add it as query arg:

esc_url( add_query_arg( $page_key, $page + 1 ) ),

I hope this context can help us take a direction ☺️

@luisherranz
Copy link
Member

Sure, what they do is reset the query, so the main query does not interfere with the query loop

I meant how does it work when the Query Loop query is inherited, because that's what we should do when the Post ID of the Comment Loop block is inherited as well.

@luisherranz
Copy link
Member

Does this make sense?

@cbravobernal
Copy link
Contributor Author

I meant how does it work when the Query Loop query is inherited because that's what we should do when the Post ID of the Comment Loop block is inherited as well.

When the Query Loop is inherited, if you mean when you are on a post or a page, the post id is got from the global object $post;

In case you have a Query Loop block, you assign a queryId in order to be able to create new queries and not be attached to that $post.

I'm not sure if I'm answering your questions 😅, we could consider creating some looms or videos if the communication feels unclear.

Does this make sense?

Maybe for the comments would be better to call it with a query word inside, in order to keep consistency with the query one. Maybe?

Right now in we have page and cpage and page as query vars. So maybe other option could be:

What do you think about it?

@DAreRodz
Copy link
Contributor

DAreRodz commented Nov 24, 2021

Uhm. 🤔 As far as I understood, the query ID would be a way to identify a specific query block, right? So, I would prefer something like cquery-X or comments-query-X, or even comments, as the name of the block is Comments Query Loop.

cpage is the query var that WordPress already uses for comment pagination, but, for me, it would only make sense in the context of a given post. Something like post-X-cpage-2. The problem with this approach is that you may want to have different queries for the same post ID but different pagination/options/etc., so using the post ID would not work in that case.

I would go for any of these options

But the one I like the most is

@luisherranz
Copy link
Member

luisherranz commented Dec 6, 2021

I forgot that the post id could be inherited from either the post/page or a parent Query Loop block and those cases should be treated differently.

Additional questions:

  • What is the behavior when there's no parent context (post/page or Query Loop) and no selected Post ID? Show the Latest Comments?
  • Is there any other source of post ids?

@DAreRodz
Copy link
Contributor

DAreRodz commented Feb 2, 2022

After some analysis, and considering the last cases @luisherranz added in the previous comment, this is how I think adding several comment loop blocks with different options should work.

Let's first consider two types of URLs:

The criteria for whether one or the other type of URLs should be used would be as follows:

  1. All the comment loop blocks which their post_id (either from context or attribute) coincide with the global post_id AND are not overwriting the WP discussion settings with custom settings (i.e., are like the usual WP comment loop), should use “Pretty” and plain permalinks.
  2. Any other case (e.g., custom per_page attribute, a different post_id, etc.) should use Block ID query var.

This would cover pretty much all the cases, and would have backwards compatibility as well, keeping the current behavior of WordPress and allowing different comment loops pagination at the same time.

This diagram shows all different cases:

image
Excalidraw link

Include the Query Loop query reference as well

From my point of view, it would not be necessary to include both the query ID and the comment query ID, with the second one would be enough. 🙂

  • What is the behavior when there's no parent context (post/page or Query Loop) and no selected Post ID? Show the Latest Comments?

I don't know if we have agreed in other issue, but pretty much yes. 👍

  • Is there any other source of post ids?

There is no other source of post IDs AFAIK.

Alright, @luisherranz @SantosGuillamot @c4rl0sbr4v0, let me know what you think. 😊

@luisherranz
Copy link
Member

LGTM! 🙂

@cbravobernal
Copy link
Contributor Author

Looks great. Also, it will be better for the UX as they will be able to add as many comments query loops as they want.

@cbravobernal
Copy link
Contributor Author

Closed as it has been paused during a long time. Feel free to reopen if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Design Feedback Needs general design feedback. Needs Technical Feedback Needs testing from a developer perspective. [Type] Discussion For issues that are high-level and not yet ready to implement. [Type] Question Questions about the design or development of the editor.
Projects
None yet
Development

No branches or pull requests

4 participants