Comment Template: Fix comment pagination with nested replies. #38187
Conversation
DAreRodz
added
[Type] Enhancement
|
Size Change: +1.5 kB (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
DAreRodz
force-pushed
the
update/comment-template-pagination-with-replies
branch
from
1f8011e to
39a9318
Compare
|
Hey, just a quick update here. Things are not properly working yet --there are some discrepancies between the editor and the frontend versions--, but these are some things I've done so far:
Again, this is not working yet, I messed up something with the comments order, but it's not far from working. 😄 And I still have to add a "show more" button if users want to show more than 2 levels of nested comments, still figuring out a proper way of doing so. |
|
I ll have a deeper look tomorrow, but awesome work so far! 🚀 |
|
Oh, after some investigation, I think we are not using the That option does not define the order in which the comments should be queried, but shown in the comment list. Looking at what the PHP template does, inside the /wp-includes/comment-template.php#L2238-L2240 if ( null === $parsed_args['reverse_top_level'] ) {
$parsed_args['reverse_top_level'] = ( 'desc' === get_option( 'comment_order' ) );
}The /wp-includes/class-wp-walker.php#L361-L362 if ( ! empty( $args[0]['reverse_top_level'] ) ) {
$top_level_elements = array_reverse( $top_level_elements );Using |
Yes, you might be correct. There was always something off when I was testing it in the past 🙈
Does it mean that |
Maybe one reason could be this:
Anyway, I completely missed the |
| // Generate a tree structure of comment IDs. | ||
| const { commentTree } = useCommentTree( | ||
| order === 'desc' | ||
| ? topLevelComments?.slice().reverse() |
There was a problem hiding this comment.
Minor note, I think we typically prefer array spread over .slice() for copying:
| ? topLevelComments?.slice().reverse() | |
| ? [ ...topLevelComments ].reverse() |
(Not that it's a hard-and-fast rule 😅 )
It would look a bit less nice if we have to take care of the null case, but maybe we can move the commentTree definition below the early return?
There was a problem hiding this comment.
Yeah, sure! The only reason I opted for that syntax was that topLevelComments could not exist, but I can refactor the code to avoid that case. I prefer array spread as well. 😆
There was a problem hiding this comment.
Awesome work @DAreRodz ! 👏
Just tested it and it works well! Do you have an idea on how you want to handle comments more than 2 levels deep?
| const [ children ] = _embedded?.children || []; | ||
| return { | ||
| commentId: id, | ||
| children: children?.map( ( child ) => ( { |
There was a problem hiding this comment.
Nitpick: no need for the ?. - children is never falsy because of the _embedded?.children || []; above.
Not yet, I haven't figured out a good way to solve that. Any idea is more than welcome. 😄 |
Status update
|
Maybe you prefer to open a new issue just for that improvement. That way we can land the comment pagination improvement. |
Oh yes, that would be better than waiting for that idea to be implemented and tested before merging these changes. 💯 |
DAreRodz
force-pushed
the
update/comment-template-pagination-with-replies
branch
from
26d56b4 to
91e7a7a
Compare
DAreRodz
requested review from
TimothyBJacobs,
spacedmonkey,
ajitbohra,
mkaz and
ryanwelcher
as code owners
| const defaultPageOptions = [ | ||
| { | ||
| label: __( 'Newest' ), | ||
| value: 'newest', | ||
| }, | ||
| { | ||
| label: __( 'Oldest' ), | ||
| value: 'oldest', | ||
| }, | ||
| ]; | ||
|
|
There was a problem hiding this comment.
Right now on Discussion Settings say:
Last or First page to display by default.
This could make confusion to the user, although, to be honest, Discussion settings are also confusing 😓
How could we improve the naming?
There was a problem hiding this comment.
I have the same feeling. I ended using oldest and newset because those are the values used internally, but I'm not sure which option is clearer from the user's perspective. For me, it's like using first or last instead would be equivalent, although it doesn't give you information about what comments are going to be shown (oldest comments are in the first or the last page? 🤷 ).
There was a problem hiding this comment.
Took it for a spin:
I don't see any "Last or First page to display by default" on this page:
Am I looking in the wrong place?
Right now it says "Default page", that seems okay to me. It's not super clear, but it's also a rare change to tweak. I'd be happy to start simple and then we can rephrase if it becomes necessary, for example to add help text.
There was a problem hiding this comment.
It's all very confusing because and I'm still not sure how it exactly works 🤣
I think it's the following:
- When you select the
lastpage displayed by default then you sort all top-level comments by date where the newest comments are on the first page. - When you select the
oldercomments at the top of each page, then it reverses the order of comments but only on individual pages.
So maybe we should have:
- Order pages by...
- Order items on the page by...
There was a problem hiding this comment.
Is it definitely necessary that a single comment loop can override the global discussion settings? 🤔 Comments are a fairly universal concept – doesn't it seem unlikely that a site would want a different treatment between post and page comments?
I worry that this could encourage folks to customise all comment loops, at which point the global settings don't actually do anything. That could lead to some confusing UX.
If we're committed to this change, should we update the language on the global discussion settings to clarify that the ordering and default page options are defaults, and might be overwritten by the theme / template?
There was a problem hiding this comment.
Comments are a fairly universal concept – doesn't it seem unlikely that a site would want a different treatment between post and page comments?
You're right, although it would be great to also consider other cases, like adding a list of “latest comments” somewhere in your page, or “top comments”, or whatever…
Well, now I'm not sure if it would be better to create custom blocks for those specific cases. 🤔
If we're committed to this change, should we update the language on the global discussion settings to clarify that the ordering and default page options are defaults, and might be overwritten by the theme / template?
Wouldn't it be simpler if we warn in the block settings that you're overwriting the global discussion settings? Just asking, I don't have any strong opinion here. 😄
There was a problem hiding this comment.
You're right, although it would be great to also consider other cases, like adding a list of “latest comments” somewhere in your page, or “top comments”, or whatever…
That's a good point, even held up against Jay's also excellent point. A few thoughts:
- Perhaps the toggle should be renamed "Inherit discussion settings", and we add help-text below that says something like "Use globally defined sort order and comment settings. You can configure these in [link]Discussion Settings[/link]".
- We start with less: perhaps we start with just "Items per page", and then instead of two options for oldest vs. newest, we make a single toggle: "Show newest comments first", untoggled by default. We'd then use this setting for both "Order by" and "Default page".
Keeping in mind we can always make it more granular as we go, it seems like this might also be an opportunity to simplify.
There was a problem hiding this comment.
- [...] and we add help-text below that says something like "Use globally defined sort order and comment settings. You can configure these in [link]Discussion Settings[/link]".
That's great, I really like that suggestion. 😊
- [...] we make a single toggle: "Show newest comments first", untoggled by default. We'd then use this setting for both "Order by" and "Default page".
Yup, that would be simpler from the user perspective, and we can always add more features/complexity in the future.
I don't mind creating new PRs to address these changes, if we all agree on that. 👀
There was a problem hiding this comment.
Well, now I'm not sure if it would be better to create custom blocks for those specific cases.
Yes I considered things like Reviews in WooCommerce as well (which are just WP comments). They feel like variations of the comment query block to me. Similar to how the "Posts List" block is a variation of the Query block.
Wouldn't it be simpler if we warn in the block settings that you're overwriting the global discussion settings?
I don't think so, from the UX perspective. To be clear I'm thinking of the other side of this flow. Consider an end user who installs a theme that elects not to inherit the global settings in each instance of the comment query loop block. When that user customises options in the global discussion settings, their changes will not be reflected on the frontend, and they will have no feedback explaining why.
There was a problem hiding this comment.
Consider an end user who installs a theme that elects not to inherit the global settings in each instance of the comment query loop block. When that user customises options in the global discussion settings, their changes will not be reflected on the frontend, and they will have no feedback explaining why.
That's a good point, I haven't thought of that. 🤔
I think it's better to continue this conversation in a different issue, I've created one: #38864
cbravobernal
added
[Package] Block library
cbravobernal
changed the title
Description
Closes #37153
Improve the Comment Template block to simplify pagination handling when nested comments are active. It uses both the WordPress discussion settings and the block props.
The implementation extends the REST API to mark comment children as 'embeddable', and only shows two levels of nested comments. Showing more than two levels will be addressed in another PR.
There are some bugs that should be addressed as well (I'll open issues for them):
Comments-Pagination-NextandComments-Pagination-Previousblocks are using WP functions that use the WP Discussion Settings under the hood, rendering a wrong link when the default page attribute of the Comment Loop block doesn’t coincide with the corresponding WP setting.How has this been tested?
Editor, with context
With.context.mov
Editor, without context
Without.context.mov
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).