Fix comments query loop pagination not respecting Discussion Settings #40284

tomasztunik · 2022-04-12T22:29:25Z

What?

Fixes so that Comments Query Loop block respects Discussion settings if comments pagination is disabled.

Why?

Currently, if in the Discussion Settings paginating comments was not enabled PHP on the front-end would correctly render all comments and would not render the pagination. In the editor, these settings were not respected. Pagination was rendered and the set per_page value would be reflected in the number of rendered comments.

How?

On the editor side, I've added checking if paginating comments was enabled and if so I made the comments-pagination/editor not render and made sure that

Gotchas! 💣 💥

Custom query on the backend has no problem with fetching any number of comments. On the editor side though we were using WP REST endpoint to fetch comments and it has a hard limit of max 100 comments per page. We've decided after a conversation on #core-editor Slack that for the purpose of layout design and setup this limit should be enough and we shouldn't aggregate all pages to render all possible data.
We shouldn't remove the pagination from the template if pagination was disabled at the time of adding the block as it wouldn't show up after the page/post was saved and comments pagination was enabled later. - this was my first take...
In theory Discussion settings allow setting more than 100 items per page. This would fail in the Editor as if we tried to fetch from WP REST API more than 100 items per page it would throw an error but would work alright on the editor. - added a guard for that

Testing Instructions

Follow the steps from Comments Query Loop is paginated in editor when pagination is not enabled #39444
With disabled comments pagination in the Discussion Settings
The pagination should not show up and all comments (up to 100) should show up

cc: @c4rl0sbr4v0 @Mamaduka

+ fix possible error where if someone set more than 100 it would return zero results in the editor but work ok when rendered by the backend

tomasztunik · 2022-04-13T08:32:05Z

These CI failures don't seem to be related

Noticed a 6 hours job timeout issue that creates $$$ and CO2 overhead. I've shared more on #core-editor slack thread

michalczaplinski

Thanks @tomasztunik ! Very nice PR 👏

Mamaduka · 2022-04-14T11:56:24Z

packages/block-library/src/comments-pagination/edit.js

+	// controls. We don't want to remove them from the template so that when the user enables
+	// paging comments, the controls will be visible.
+	if ( ! pageComments ) {
+		return null;


Maybe we should explain why the editor doesn't display the actual pagination.

Added a comment 👍

Sorry for the confusion. I meant explaining to the user. Currently, this will render nothing, and folks might think it's a bug.

Ah thought it might be one of the two but this was quick enough that decided to see if it was the right call :)

Can you point me toward an example for similar conditional controls/UI elements? If we were to announce it to the user better - would it be better to just render the control but annotate it as "hidden" because of a setting, or render a warning/placeholder saying pagination would be here but it is disabled by settings?

Will roll back the comment for now.

The only thing that comes to mind is the Reusable block message that it cannot be rendered inside itself 😅 We can probably skip the warning wrapper.

gutenberg/packages/block-library/src/block/edit.js

Line 83 in 0070d20

{ __( 'Block cannot be rendered inside itself.' ) }

Do you know who was leading design efforts for the Comment blocks? Maybe they have suggestions as well.

I've pushed the code but now I'm thinking... should "Discussion Settings" be a link? :)

Sorry, not sure about that.

I'm not 100% convinced that we should add the <Warning /> here if the lack of pagination is the expected and correct behaviour. I would lean towards not rendering any additional UI elements.

I think it might be confusing if we use the same UI element when there is an actual error (e.g. when we show the This block has encountered an error and cannot be previewed) and also use this element to just inform the user why no pagination is being displayed.

@jameskoster was involved in the design of some of the Comment blocks so perhaps he'd like to chime in?

I've had a quick look and the <Warning /> component seems to be used in a couple of other blocks including comments related blocks to inform about data availability and settings state. Ie. PostCommentsForm. The message though could be aligned with these as they all include the block name with a message following it.

I think there is value in the warning, and semantically it seems correct if we assume that the warning is not for the user who disabled the pagination but rather a warning to let other users (or the future "me") working with the blocks about why the pagination is not there as there is no obvious way to learn that without remembering to go to settings to review that.

waiting for more insights here :)

(updated the copy to match how warnings are used in other comments related blocks)

Yeah, that makes sense. I withdraw my objections since there is precedent for using the <Warning/> to inform rather than show errors as you point out. 🙂

Re: the copy - I think it makes sense if we link to the Discussion settings. The only problem is that I don't see a reliable way to determine the admin URL. There was something custom added to that effect as a global window.wpAdminManifestL10n which is then used here but it's an experimental package and we can't rely on it.

packages/block-library/src/comments-pagination/edit.js

Co-authored-by: George Mamadashvili <[email protected]>

this is to enable styling the controls regardless of data and state

This will take place of the pagination placeholder. + Remove unneeded comment about comments pagination being a placeholder.

cbravobernal · 2022-04-18T08:36:30Z

Thanks for the PR!

Are 100 results a lot to show in the editor?

This could affect the performance and the editing experience. I guess there won't be many cases of users adding this out of the Site Editor (like in a post or page), but I guess that limiting the results to about 10 would be a good number.

tomasztunik · 2022-04-18T12:29:12Z

Good point, but then if there are is more data should we show a warning that results are limited in the editor? pon., 18 kwi 2022, 10:36 użytkownik Carlos Bravo ***@***.***> napisał:

…

Thanks for the PR! Are 100 results a lot to show in the editor? This could affect the performance and the editing experience. I guess there won't be many cases of users adding this out of the Site Editor (like in a post or page), but I guess that limiting the results to about 10 would be a good number. — Reply to this email directly, view it on GitHub <#40284 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAA3NDX2PKHCCXWY36A34GLVFUNJTANCNFSM5TIXCW7Q> . You are receiving this because you were mentioned.Message ID: ***@***.***>

cbravobernal · 2022-04-18T14:39:55Z

Good point, but then if there are is more data should we show a warning
that results are limited in the editor?

Another option could be to show that information on the block description. I'm not sure if always showing a warning is a good UX. Let's add a needs design label and mention it on the Slack channel.

cbravobernal · 2022-04-19T11:49:29Z

Answer from design contributors, link to slack:

Hey, good question, thanks for bringing it up.
In this case, there’s a similar issue with the regular (post) query loop, which also limits the amount shown in the editor (and even list view). As a baseline, I’d do the same for the comments query loop if you’re able to.
In both cases, we could add some help text indicating that the visual display is limited in the editor only, and why. But I’d add this help text to the inspector, potentially under the block description. I’d also do it as a separate followup PR, just since it hasn’t been problematic for the query loop.

Let's address it in another PR @tomasztunik , thanks for the contribution!

…#40284) * hide comments pagination in the editor if not enabled * if comments pagination not enabled fetch max allowed items + fix possible error where if someone set more than 100 it would return zero results in the editor but work ok when rendered by the backend * update selector to return only used data Co-authored-by: George Mamadashvili <[email protected]> * clarify that in editor we are rendering pagination placeholder this is to enable styling the controls regardless of data and state * Present a user facing warning if comments pagination was disabled. This will take place of the pagination placeholder. + Remove unneeded comment about comments pagination being a placeholder. * align disabled comments warning copy with other comments blocks Co-authored-by: George Mamadashvili <[email protected]>

gziolo · 2022-04-25T14:46:08Z

Cherry-picked to the wp/6.0 branch for WordPress Beta 3 release.

tomasztunik added 2 commits April 13, 2022 00:10

hide comments pagination in the editor if not enabled

45943f4

if comments pagination not enabled fetch max allowed items

ca311a0

+ fix possible error where if someone set more than 100 it would return zero results in the editor but work ok when rendered by the backend

tomasztunik requested review from michalczaplinski and ajitbohra as code owners April 12, 2022 22:29

cbravobernal added the [Block] Comments Affects the Comments Block - formerly known as Comments Query Loop label Apr 13, 2022

michalczaplinski approved these changes Apr 14, 2022

View reviewed changes

Mamaduka reviewed Apr 14, 2022

View reviewed changes

packages/block-library/src/comments-pagination/edit.js Outdated Show resolved Hide resolved

tomasztunik and others added 4 commits April 14, 2022 14:39

update selector to return only used data

40ecf5d

Co-authored-by: George Mamadashvili <[email protected]>

clarify that in editor we are rendering pagination placeholder

31718f6

this is to enable styling the controls regardless of data and state

Present a user facing warning if comments pagination was disabled.

250730c

This will take place of the pagination placeholder. + Remove unneeded comment about comments pagination being a placeholder.

align disabled comments warning copy with other comments blocks

85fc1aa

cbravobernal added the Needs Design Feedback Needs general design feedback. label Apr 18, 2022

cbravobernal requested a review from beafialho April 18, 2022 14:43

cbravobernal merged commit f345992 into WordPress:trunk Apr 19, 2022

cbravobernal added [Type] Bug An existing feature does not function as intended and removed Needs Design Feedback Needs general design feedback. labels Apr 19, 2022

github-actions bot added this to the Gutenberg 13.1 milestone Apr 19, 2022

adamziel added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 19, 2022

cbravobernal mentioned this pull request Apr 19, 2022

Create comments title with simple styling #40419

Merged

7 tasks

ockham mentioned this pull request Apr 20, 2022

Comments Loop Block: Tracking issue #34994

Closed

13 tasks

gziolo removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 25, 2022

femkreations added the Needs User Documentation Needs new user documentation label Aug 22, 2022

femkreations mentioned this pull request Oct 5, 2022

[HelpHub] Comment Pagination block WordPress/Documentation-Issue-Tracker#491

Closed

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix comments query loop pagination not respecting Discussion Settings #40284

Fix comments query loop pagination not respecting Discussion Settings #40284

tomasztunik commented Apr 12, 2022 •

edited

Loading

tomasztunik commented Apr 13, 2022

michalczaplinski left a comment

Mamaduka Apr 14, 2022

tomasztunik Apr 14, 2022

Mamaduka Apr 14, 2022

tomasztunik Apr 14, 2022

Mamaduka Apr 14, 2022

tomasztunik Apr 14, 2022

Mamaduka Apr 14, 2022

michalczaplinski Apr 14, 2022

tomasztunik Apr 15, 2022 •

edited

Loading

michalczaplinski Apr 15, 2022

cbravobernal commented Apr 18, 2022

tomasztunik commented Apr 18, 2022 via email

cbravobernal commented Apr 18, 2022 •

edited

Loading

cbravobernal commented Apr 19, 2022

gziolo commented Apr 25, 2022

Fix comments query loop pagination not respecting Discussion Settings #40284

Fix comments query loop pagination not respecting Discussion Settings #40284

Conversation

tomasztunik commented Apr 12, 2022 • edited Loading

What?

Why?

How?

Gotchas! 💣 💥

Testing Instructions

tomasztunik commented Apr 13, 2022

michalczaplinski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomasztunik Apr 15, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbravobernal commented Apr 18, 2022

tomasztunik commented Apr 18, 2022 via email

cbravobernal commented Apr 18, 2022 • edited Loading

cbravobernal commented Apr 19, 2022

gziolo commented Apr 25, 2022

tomasztunik commented Apr 12, 2022 •

edited

Loading

tomasztunik Apr 15, 2022 •

edited

Loading

cbravobernal commented Apr 18, 2022 •

edited

Loading