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

Comment Template block: Handle nested comments #36065

Conversation

michalczaplinski
Copy link
Contributor

@michalczaplinski michalczaplinski commented Oct 29, 2021

Description

This is a follow-up PR to #35231 which adds handling of nested comments.

How has this been tested?

  1. Install and activate a FSE theme (like the "TT1 Blocks" ).
  2. Create a new post.
  3. Add some comments to the new post, including some nested comments.
  4. Edit the post and insert the Comments Query Loop block .
  5. Check that the replies are indeed nested under the their respective "parent" comments.

Screenshots

admin

Screen Shot 2021-11-25 at 20 06 30

frontend

Screen Shot 2021-11-25 at 20 07 06

Types of changes

This is still a pretty barebones implementation of comment nesting.

Styling is very basic and will be tackled in another PR. For now, I've just included enough so that you can visually discern the nested comments by their indentation.

Also:

  • The list of comments is now a ol instead of ul element because comments are an ordered list
  • There are still some glitches in the UI as you attempt to navigate / switch focus between the contents of the Comment Template - sometimes the blocks do not get "active" immediately. Will also handle it in another PR.
  • I've removed the code that was supposed to inherit settings from the Discussion settings that was present in the early versions of this PR for reasons described below in Comment Template block: Handle nested comments #36065 (comment)

@gziolo gziolo added [Block] Comment Template Affects the Comment Template Block [Type] Enhancement A suggestion for improvement. labels Oct 29, 2021
@michalczaplinski
Copy link
Contributor Author

newmovie.mov

I wonder what the best UX here should be. Like I mention in the video, if this setting is set:

Screen Shot 2021-10-29 at 18 16 04

we cannot easily fetch the X top level comments AND it's replies.

I think it would be pragmatic to (for now) ignore the "Break comments into X pages with top level comments per page..." setting and just provide a default number of comments per page in block.json (let's say 50) Would you agree @SantosGuillamot @gziolo ?

@gziolo
Copy link
Member

gziolo commented Nov 2, 2021

I think it would be pragmatic to (for now) ignore the "Break comments into X pages with top level comments per page..." setting and just provide a default number of comments per page in block.json (let's say 50) Would you agree @SantosGuillamot @gziolo ?

In my opinion, we can land all the improvements that are ready, and work on the rest of the follow-up PRs. I don't mind if you leave the one that seems the more complex for the end. I don't think we must provide all the same functionalities if they don't make too much sense in the block-based world.

@gziolo
Copy link
Member

gziolo commented Nov 2, 2021

There are some minor issues with coding conventions in the SCSS file:

Screen Shot 2021-11-02 at 12 31 11

I also see that the test fixtures for the Comment Template block need to be regenerated after the default value for comments per page was removed.

@SantosGuillamot
Copy link
Contributor

I think it would be pragmatic to (for now) ignore the "Break comments into X pages with top level comments per page..." setting and just provide a default number of comments per page in block.json (let's say 50) Would you agree @SantosGuillamot @gziolo ?

I agree that if the settings are still available from the block it isn't a big deal to not follow the global setting from the beginning. Maybe we can create a follow-up issue for future reference.

@michalczaplinski michalczaplinski force-pushed the update/comments-query-loop-add-features branch from c0dcc4f to cc92c87 Compare November 8, 2021 23:51
@michalczaplinski michalczaplinski force-pushed the update/comments-query-loop-add-features branch 2 times, most recently from af4a3d9 to 6b91b87 Compare November 24, 2021 02:24
@SantosGuillamot SantosGuillamot linked an issue Nov 24, 2021 that may be closed by this pull request
@michalczaplinski michalczaplinski force-pushed the update/comments-query-loop-add-features branch from 172628b to b3b00c0 Compare November 26, 2021 00:46
@michalczaplinski michalczaplinski changed the title Comment Template block: Fixes and improvements Comment Template block: Handle nested comments Nov 26, 2021
@michalczaplinski michalczaplinski marked this pull request as ready for review November 26, 2021 01:34
@gziolo
Copy link
Member

gziolo commented Nov 29, 2021

@michalczaplinski, that's a complex case to support those nested comments. I think it's on track. Really good work sorting out all the complexity comment threads bring. I left some initial comments mostly around code guidelines that are important for blocks because those PHP files get copied unmodified to the WordPress core once moved out of the experimental stage.

@michalczaplinski michalczaplinski force-pushed the update/comments-query-loop-add-features branch from 3afc014 to d3a0183 Compare November 29, 2021 23:14
@michalczaplinski michalczaplinski force-pushed the update/comments-query-loop-add-features branch from ccd8529 to 802e379 Compare December 3, 2021 17:56
@gziolo
Copy link
Member

gziolo commented Dec 6, 2021

There is some issue with block focus/selection, related to what you mentioned in your last point. Sometimes, you have to click twice or three times in order to select the relevant block inside of the Template. And sometimes. even then, the an incorrect block gets selected initially.

I'm following the progress on #36431 where @andrewserong is trying to resolve similar issues for the Query Loop block. I don't know if those are the same exact cases, but given that we use nearly the same code we might have a solution quite soon. Let's file an issue for now and watch the progress on the linked PR.

I addressed some of my nitpicks that don't impact the functionality. Let's land this PR and continue in follow-up PRs. I will open issues for the things discovered during testing.

@gziolo
Copy link
Member

gziolo commented Dec 6, 2021

Improve the way REST API is handled for threaded comments as discussed above.

Filed in #37153.

Selecting blocks for nested comments sometimes changes the position on the screen in an unexpected way, you can observe that in the video I included.

Filed in #37154.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Comment Template Affects the Comment Template Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comments Query Loop block
5 participants