Comments Query Loop: Add discuss options to the block #37297
Conversation
cbravobernal
added
[Block] Comments
cbravobernal
requested review from
gziolo,
michalczaplinski,
DAreRodz and
luisherranz
and removed request for
ajitbohra
|
Size Change: +294 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
|
||
| export default function CommentsQueryOrderControls( { value, onChange } ) { | ||
| return ( | ||
| <ToggleGroupControl |
There was a problem hiding this comment.
It doesn't seem that the ToggleGroupControl is the right one for this control.. I think a combo box control is a better fit here and will accommodate for more options, if they need to be added.
There was a problem hiding this comment.
Thanks for the review!!
It seems that order options only accepts two values here, a long time ago for now. This is what we have when we check the documentation about the comments arguments on that function.
Maybe the combo box would be a better option for OrderBy?
There was a problem hiding this comment.
I think the common case is to combine order and orderBy in a single control when we want both, but if we want only a control with two values (asc, desc) here, we might even consider a simple toggle control? --cc @jasmussen
There was a problem hiding this comment.
Nice PR! Yes, as a rule of thumb, if the text labels wrap inside a segmented control, then that control is not the right choice. I would also lean towards a basic toggle in cases where there are only two options. The combobox/dropdown is good for surfacing longer labels and obviously showing many options.
Is the toggle right for sort order? I'd say that depends on the phrasing and if it feels right. Something like this might work:
[on] Sort from newest to oldest
The implication would be that toggling it off would sort from oldest to newest. I think I'd personally like this as a good starting point, since it's a fairly simple toggle and good baseline.
We can always upgrade to the dropdown, which could be something like:
Sort order:
[ Newest to oldest
Oldest to newest ]
What do you think?
cbravobernal
changed the title
| @@ -46,6 +47,12 @@ export default function CommentsQueryLoopEdit( { attributes, setAttributes } ) { | |||
| setAttributes( { tagName: value } ) | |||
| } | |||
| /> | |||
| <QueryOrderControls | |||
There was a problem hiding this comment.
This setting should probably be located outside of the advanced group, in their own group specific for the block.
There was a problem hiding this comment.
Added on the toolbar. But I don't like how it shows. Maybe we could consider moving it to a Settings Inspector Options panel.
There was a problem hiding this comment.
It can be in the sidebar. It just shouldn’t be inside the advanced panel where controls related to tweaking HTML code live.
cbravobernal
force-pushed
the
update/add-comments-query-loop-sorting-option
branch
2 times, most recently
from
dc4f4e3
to
25ab9be
Compare
| // We cannot get the order of the discussion settings page. | ||
| // Without the order defined, the default on the API is DESC | ||
| // with means show the newest comments first. | ||
| // By default, in discussion settings, the order is ASC. | ||
| // So the frontend is showing older comments first, while the editor is showing newer ones. | ||
| if ( order ) { | ||
| commentQuery.order = order; | ||
| } |
There was a problem hiding this comment.
I'm not confident with this solution, as both editor and frontend differ by default.
What is your opinion about this?
Could we solve it with the toggle box of inherit from settings proposal?
That way, the order would be:
- If inherit enabled: settings
- If inherit disabled: DESC (newer first) by default like the REST API, otherwise chosen by the user with the toggle.
cc: @gziolo @ntsekouras @jasmussen @michalczaplinski @DAreRodz
There was a problem hiding this comment.
We should read the default values from the server and use them when the user selects no explicit override. I think @michalczaplinski, at some point, had some code that exposes the site settings through the REST API. Am I correct, or was it something else?
There was a problem hiding this comment.
Yes! I wanted to have comments_per_page exposed in the REST API. Here are quite complete instructions on how to do it, they should apply to the order property as well:
https://wordpress.slack.com/archives/C02QB2JS7/p1635376326175900
For those without access to the WP slack, here's how it can be done. It should be added to /lib/rest-api.php
function register_block_core_comment_template_order_setting() {
register_setting(
'discussion',
'order',
array(
'show_in_rest' => true,
)
);
}
add_action( 'rest_api_init', 'register_block_core_comment_template_order_setting', 10 );There was a problem hiding this comment.
Wow, this is a really quick way to fix it, it will save me a lot of time 🎉
Would it be a good idea to create an endpoint to retrieve all the discussion settings? We could use register_rest_route, get all Discussion settings and use them for all the block rendering.
There was a problem hiding this comment.
I don't think you need to create a new endpoint for that. As far as I'm aware the code that @michalczaplinski shared, adds the setting to the endpoint that is already in use in Gutenberg.
There was a problem hiding this comment.
I have an update in regards to these settings. I talked to @spacedmonkey and he explained that the https://make.wordpress.org/core/wp-json/wp/v2/settings endpoint is only available for admins. There is a long-standing Tract ticket that we would need to land: https://core.trac.wordpress.org/ticket/48885 – the idea is to expose site settings in the read-only mode for all user roles.
As a short-term solution, we could always expose those setting using the block_editor_settings_all filter. See am example of how to share the settings from the server:
gutenberg/lib/compat/wordpress-5.9/default-editor-styles.php
Lines 17 to 41 in a92586c
|
This is what we see on the toolbar right now. Is there any way to make it wider without custom CSS?
Thanks! |
I guess it will be easier to expand in the sidebar. |
|
On the sidebar is easier to handle.
The code is working, but the editor comments won't get updated if the user is not an admin. |
cbravobernal
force-pushed
the
update/add-comments-query-loop-sorting-option
branch
from
81b6ff6
to
639a420
Compare
cbravobernal
requested review from
spacedmonkey and
TimothyBJacobs
as code owners
cbravobernal
force-pushed
the
update/add-comments-query-loop-sorting-option
branch
from
639a420
to
8018460
Compare
cbravobernal
force-pushed
the
update/add-comments-query-loop-sorting-option
branch
from
990930c
to
dc5c16b
Compare
Co-authored-by: Greg Ziółkowski <grzegorz@gziolo.pl>
cbravobernal
requested review from
ellatrix,
nerrad,
ntwb and
oandregal
as code owners
cbravobernal
force-pushed
the
update/add-comments-query-loop-sorting-option
branch
from
8f4924c
to
67bfc8e
Compare
There was a problem hiding this comment.
Let's ship this version. I think the way it works now is pretty good. Everything works correctly in the case settings are inherited from the site settings and when some custom values get selected. Toggling settings inheriting also works great.
I'm testing using the legacy Post Comments block next to the Comments Query Loop block added to the Single Post template. I noticed that the order of pages differs between those two. I bet we need some more legacy logic related to the mix of first/last page and older/newer settings:
Let's look into it the follow-up PR.
cbravobernal
deleted the
update/add-comments-query-loop-sorting-option
branch
| .block-library-comments-toolbar__popover .components-popover__content { | ||
| min-width: 230px; | ||
| } |
There was a problem hiding this comment.
While working on #40522, @DAreRodz, @michalczaplinski and I found that this file doesn't seem to be used.
It seems that it was introduced to fix the issue mentioned here, but it appears the relevant settings were later moved to the inspector controls in the sidebar, making the styling obsolete.
Just documenting this here for posterity 😄
Description
In this PR, we are adding the option to sort the comments on the Query Loop by Newer/Older.
A review of the copies is kindly appreciated 😄 .
How has this been tested?
Screenshots
Loom Video:
Edit Post “Post sample” ‹ gutenberg — WordPress - 20 January 2022 - Watch Video
Types of changes
New feature, adds an attribute to Comment Query Loop.
Checklist:
*.native.jsfiles for terms that need renaming or removal).