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

Site Editor: Add new Comments Query Loop block #35231

Merged

Conversation

michalczaplinski
Copy link
Contributor

@michalczaplinski michalczaplinski commented Sep 30, 2021

Description

Fixes #34996
Fixes #35000

  • This PR is adding a new Comments Query block to block-library.
  • It includes only the bare skeleton of the block without all the attributes that should be present in the final version.
  • I've included a stub implementation of the core/comment-template block which is kept to the bare minimum to make the comments query work.

How has this been tested?

Tested manually:

  1. Create a post
  2. Type /comment
  3. Select the Comment Query Loop block
  4. Change the perPage, pages or offset attributes in the toolbar and observe them change in the UI

Environment details

  • WordPress Version: 5.9-alpha-51831
  • Database: wp-env default
  • PHP Version: wp-env default
  • Node: 14.17.6
  • npm: 6.14.15

Screenshots

Screen Shot 2021-10-07 at 20 09 01

Screen Shot 2021-10-07 at 20 09 35

Screen Shot 2021-10-07 at 20 10 57

Screen Shot 2021-10-07 at 20 13 45

Types of changes

  • Type: Experimental
  • Needs Testing
  • New feature (Block Editor)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @michalczaplinski! In case you missed it, we'd love to have you join us in our Slack community, where we hold regularly weekly meetings open to anyone to coordinate with each other.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 30, 2021
@michalczaplinski michalczaplinski changed the title [WIP] Block Editor: Add new Comments Query Loop block [WIP] Site Editor: Add new Comments Query Loop block Sep 30, 2021
@michalczaplinski michalczaplinski force-pushed the add/post-comment-query-loop-block branch from 1f39143 to 16ee987 Compare October 6, 2021 21:31
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Query Loop is quite a big block with several functionalities added over time. It took months to develop it. I guess the best approach would be to start simple and code only a small subset of features. @ntsekouras might have the best recommendation here. Definitely, a good start would be to have a Comments Query Loop and Comment template block with some enforced layout for start with simple controls that change some common query attributes.

packages/block-library/src/comments-query/edit/index.js Outdated Show resolved Hide resolved
@michalczaplinski michalczaplinski changed the title [WIP] Site Editor: Add new Comments Query Loop block Site Editor: Add new Comments Query Loop block Oct 8, 2021
Copy link
Contributor Author

@michalczaplinski michalczaplinski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the comments @gziolo! I've reduced the scope of this PR to remove the variations, and CommentsQuerySetup and kept just the 3 attributes: perPage, pages and offset for now.

I would appreciate some more eyes on this PR - perhaps @ntsekouras also could take a look to let me know what else you would like to see here in order for this PR to land?

packages/block-library/src/comments-query/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/comments-query/index.js Outdated Show resolved Hide resolved
packages/block-library/src/comments-query/toolbar.js Outdated Show resolved Hide resolved
@michalczaplinski michalczaplinski marked this pull request as ready for review October 8, 2021 01:22
@michalczaplinski
Copy link
Contributor Author

No idea why e2e tests are failing but it seems unrelated...? 🤔

@michalczaplinski
Copy link
Contributor Author

I think this block should have e2e tests similar to ones for the query block, right?

@ntsekouras
Copy link
Contributor

Thanks for working on this @michalczaplinski - this is awesome!

I'll take a closer look later to review properly but for now I was just wondering if it makes sense to revisit the design/position of the controls. On one hand it would be good to be consistent between similar blocks, on the other hand if the attributes are fewer, we could move them to the Inspector controls maybe? We have a long standing issue about Query Loop controls and very often having the controls in the toolbar has come up as a source of confusion for users.

--cc @jameskoster @jasmussen

Copy link
Contributor

@ntsekouras ntsekouras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this Michal!

This is probably a chance for us to enforce all the new stuff (like layout) and handle better some challenges that we faced with Query Loop. In general I think more things to discuss will be exposed when you start the implementation of Comments Loop client and server side.

packages/block-library/src/comments-query/block.json Outdated Show resolved Hide resolved
packages/block-library/src/comments-query/save.js Outdated Show resolved Hide resolved
packages/block-library/src/comments-query/toolbar.js Outdated Show resolved Hide resolved
packages/block-library/src/comments-query/toolbar.js Outdated Show resolved Hide resolved
packages/block-library/src/comments-template/block.json Outdated Show resolved Hide resolved
packages/block-library/src/index.js Show resolved Hide resolved
packages/block-library/src/comments-query/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/comments-query/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/comments-query/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/comments-query/edit.js Outdated Show resolved Hide resolved
@jameskoster
Copy link
Contributor

We have a long standing issue about Query Loop controls and very often having the controls in the toolbar has come up as a source of confusion for users.

I would be in favour of maintaining consistency across the query blocks for now, then addressing the issues of the current UX subsequently and applying any fixes to all blocks at the same time.

@michalczaplinski michalczaplinski force-pushed the add/post-comment-query-loop-block branch from 1bca6cc to 8fe3842 Compare October 19, 2021 01:53
@gziolo gziolo added [Block] Comments Affects the Comments Block - formerly known as Comments Query Loop New Block Suggestion for a new block labels Oct 19, 2021
@gziolo
Copy link
Member

gziolo commented Oct 19, 2021

We need to sort out the execution plan before we land this PR. At the moment there are two existing blocks:

  • core/post-comments
  • core/post-comment

They are going to be replaced with:

  • 'core/comments-query'
  • core/comments-template'

Now the question is how we approach that:

  1. Do we keep both versions of blocks for some time? It would require a manual update for all existing block themes that use core/post-comments or core/post-comment.
  2. Do we keep old (internal) block names and replace them with the revised implementation? Is that even possible from a backward compatibility standpoint?

Do we have other options available that I missed? Is it something that you already know how to address? /cc @mtias and @youknowriad.

@youknowriad
Copy link
Contributor

Potentially, we could keep the old block only in the plugin and only backport one of them to Core. For Post, we did keep both blocks (latest posts and Query Loop).

@gziolo
Copy link
Member

gziolo commented Oct 20, 2021

Potentially, we could keep the old block only in the plugin and only backport one of them to Core. For Post, we did keep both blocks (latest posts and Query Loop).

I forgot about Latest Comments. It is there as well:

At the moment, we have the experimental Post Comments block that is very simple and seems to be a blocky version of comments_template() call in PHP:

There is also an existing experimental Post Comment block that requires providing the comment id by the user in the editor:

<TextControl
value={ commentId }
onChange={ ( val ) =>
setCommentIdInput( parseInt( val ) )
}
/>

This PR proposes a Comments Query Loop block that is a more advanced version of Post Comments. The other block Comments Template is very similar to the existing Post Comment block.

@michalczaplinski michalczaplinski force-pushed the add/post-comment-query-loop-block branch from df17429 to 0a97105 Compare October 21, 2021 01:50
@michalczaplinski
Copy link
Contributor Author

I'd like to understand what else would be needed in order to land this PR? I only have this on my todo list but there's probably more that I'm missing:

@gziolo @ntsekouras would you like to give it another look?

lib/blocks.php Outdated Show resolved Hide resolved
@michalczaplinski michalczaplinski force-pushed the add/post-comment-query-loop-block branch from f6c69a4 to 0264d67 Compare October 27, 2021 20:58
@michalczaplinski
Copy link
Contributor Author

Definitely, number set as an attribute alone might be confusing. There is an offset already, so something related to the limit (like in SQL) could also work. Naming is hard, for some folks number of comments might be read as the total number of comments. I know I'm not helping 🤷🏻 😅

Yeah, I was also thinking of limit. Naming is hard indeed. I've realized that we will add pagination later on so then queryPerPage makes more sense. Will just keep it as is for now.

@michalczaplinski
Copy link
Contributor Author

My current concern is how to handle the nested comments.

It seems to me that that the best way would be to convert the list of comments passed in blockContexts:

const blockContexts = useMemo(
() => comments?.map( ( comment ) => ( { commentId: comment.id } ) ),
[ comments ]
);

to a format that includes an array of children. This is possible because each comment object also stores an ID of its parent comment.

[
	{ commentId: 1, children: [] },
	{
		commentId: 2, children: [
			{ commentId: 3, children: [] },
			{ commentId: 4, children: [ 
			   { commentId: 5, children: [] } 
		  ] },
		],
	},
	{ commentId: 3, children: [] },
];

You get the idea.

Probably would need to split the ` into 2 components. The first one that contains the logic and the second one that "just" renders the content given some props.

Then, that second component call itself recursively (if the comment has children) so that I can represent each level of comment nesting.

@ntsekouras
Copy link
Contributor

My current concern is how to handle the nested comments.

I think the first iteration could skip this and after landing, create a quick follow up for the nested handling. What do you think?

Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome work. I think it's in good shape already so we can land it to start testing in the Gutenberg plugin. We still have almost a week before the 11.9 RC1 happens to implement more features necessary for the Comments Query Loop block. The biggest benefit of landing this PR early is that we can start deprecating the Post Comment block in favor of the Comment Template block.

@gziolo gziolo merged commit d51ac8a into WordPress:trunk Oct 28, 2021
@github-actions github-actions bot added this to the Gutenberg 11.9 milestone Oct 28, 2021
@ntsekouras
Copy link
Contributor

Great work! 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Comments Affects the Comments Block - formerly known as Comments Query Loop First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository New Block Suggestion for a new block [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comments Template block Comments Query Loop block
9 participants