Comments Query Loop: Improve context handling in inner blocks #37196
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gziolo
changed the title
|
Size Change: -2 B (0%) Total Size: 1.11 MB
ℹ️ View Unchanged
|
gziolo
force-pushed
the
update/comment-query-loop-pagination
branch
from
63ec6b4 to
be83ac6
Compare
10 tasks
gziolo
added
[Block] Comments
gziolo
force-pushed
the
update/comment-query-loop-pagination
branch
from
be83ac6 to
0d504f8
Compare
gziolo
commented
Dec 8, 2021
gziolo
commented
Dec 8, 2021
gziolo
force-pushed
the
update/comment-query-loop-pagination
branch
4 times, most recently
from
d9fcd6c to
179fc66
Compare
gziolo
commented
Dec 8, 2021
| @@ -138,13 +138,16 @@ export default function CommentTemplateEdit( { | |||
| [ postId, clientId ] | |||
| ); | |||
|
|
|||
| // TODO: Replicate the logic used on the server. | |||
| perPage = perPage || 50; | |||
There was a problem hiding this comment.
It looks like by default the REST API endpoint returns only 10 comments.
gziolo
commented
Dec 8, 2021
| "type": "number", | ||
| "default": 50 | ||
| "default": null |
There was a problem hiding this comment.
It looks like the best way to replicate the behavior from the Query Loop block:
gutenberg/packages/block-library/src/query/block.json
Lines 16 to 18 in 8aa8230
This way we still can use the site settings set for comments. The only issue I discovered is that once we change the value in the UI, it's impossible to reset it with the existing toolbar control.
With no value selected:
With the value selected:
There was a problem hiding this comment.
Would it make sense to add an option to inherit the perPage value from the WordPress settings? Something similar to the Layout settings, but in the toolbar instead of the sidebar:
Maybe for a later iteration?
There was a problem hiding this comment.
Oh, I see, that would be nice! I feel like it needs like a "(?)" disclaimer in the UI or something like that to clarify what we mean by "inherit" though 🙂
And we could "link" the inherit toggle and the perPage selector in javascript so that they they are more intuitive:
- By default the
inheritis ON. - Toggle the
inheritOFF, it sets to theperPageto a pre-defined value (e.g10). - Toggle the
inheritON, it sets theperPageselector tonull.
There was a problem hiding this comment.
@DAreRodz, I like the proposal very much, and as far as I remember it's how the Query Loop block operates. @ntsekouras, is that still true?
@c4rl0sbr4v0, so that might be the solution for using the default site setting for everything related to comments. In that scenario, we would have to make all related attributes either don't define any default value or use null as an explicit way to tell blocks to read site settings.
There was a problem hiding this comment.
@DAreRodz, I like the proposal very much, and as far as I remember it's how the Query Loop block operates. @ntsekouras, is that still true?
Yes it is and when we inherit in Query Loop we also hide some controls (maybe we should hide more though 😄 ). Another thing in Query Loop is that it initializes the perPage from settings if not explicitly provided.
gziolo
force-pushed
the
update/comment-query-loop-pagination
branch
from
179fc66 to
340fee0
Compare
|
Current status: Screen.Recording.2021-12-09.at.00.06.54.movI fixed the issue with the justification of the Comments Pagination block. The existing |
11 tasks
|
Looks great so far @gziolo! 🙂 It seems like it's near getting finished - are you planning to add tests for the EDIT: Oh, I see that you already said you ll add tests in #37196 (comment) |
13 tasks
7 tasks
gziolo
force-pushed
the
update/comment-query-loop-pagination
branch
from
340fee0 to
8a8d0e2
Compare
gziolo
commented
Dec 13, 2021
|
|
||
| public function setUp() { | ||
| parent::setUp(); | ||
|
|
||
| update_option( 'page_comments', true ); |
There was a problem hiding this comment.
It looks like it's a common pattern to update options whenever necessary in the unit test:
gziolo
requested review from
ajitbohra,
spacedmonkey and
TimothyBJacobs
as code owners
Some basic unit tests were added. Let's try to land this PR, so we can work on other site options that exist for comments like sorting that @c4rl0sbr4v0 tackles in #37297. We also need to make it possible to disable showing threads for comments. |
gziolo
force-pushed
the
update/comment-query-loop-pagination
branch
from
8a8d0e2 to
150a47e
Compare
cbravobernal
approved these changes
Dec 13, 2021
22 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
It might replace #36623. I still don't know. I'm exploring the minimal set of block context values to use to make pagination work. I'm also using this branch to document issues discovered while testing.
At the moment only the following site settings are respected:
We still need to add better support for:
I propose we:
queryIdattribute from the Comments Query Loop block and from the block context for now since it is not used. We can add it back once we have a clear vision for multiple queries on a single page discussed in Should we provide multi-query comments loops? #36642.queryPerPageattribute in the Comments Query Loop toperPageand expose it in the block context ascomments/perPage. It should help to clarify that the context is coming from the parent block and aligns more closely with the documentation that promotes namespaces: https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-context.md#defining-block-context.build_comment_query_vars_from_blockto have a unified way to query comments and all related characteristics. It should work both for pages that define a currently selected post id and all other pages where users might want to show a custom list of comments.The implementation is based on the functionality present in WordPress core in the
comments_templatefunction.How has this been tested?
Screenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).