Comments block: Change ID to core/comments
#40506
Conversation
|
Size Change: +58 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
I'd add that it also has inner blocks that go beyond a query, such as the Comments Title, and the Comments Form (which we're going to include per #40256) 😄 |
| @@ -2,7 +2,7 @@ | |||
| "$schema": "https://schemas.wp.org/trunk/block.json", | |||
| "apiVersion": 2, | |||
| "name": "core/comments-query-loop", | |||
There was a problem hiding this comment.
Let's actually also change the ID:
| "name": "core/comments-query-loop", | |
| "name": "core/comments", |
Reasoning: It's overall much less confusing to have the ID reflect the block name (and file and folder names), especially if there are plenty of related blocks with similar names.
As for the reasoning why I think we should rename the block overall, see #40506 (comment).
Let's add some logic to convert from the old to the new ID here:
gutenberg/packages/blocks/src/api/parser/convert-legacy-block.js
Lines 58 to 68 in 47a5359
There was a problem hiding this comment.
Adding that it seems like the stabilized Comments Query Loop block has so far only been in on GB plugin release (v13.0.0): 6f7c5b5
cbravobernal
requested review from
TimothyBJacobs,
spacedmonkey,
ntwb,
nerrad,
ajitbohra,
michalczaplinski,
mkaz,
ryanwelcher and
juanmaguitar
as code owners
|
Given latest developments (#40521), I'm afraid we can close this now 😅 |
ockham
force-pushed
the
update/rename-comments-query-loop
branch
from
245e472
to
cd004c4
Compare
|
Inserting a Comments Query Loop on
Console:
I think we have to update a few |
|
If we get totally stuck on the block validation error, our fallback could probably be to leave the block ID unchanged (i.e. keep it as |
| save( { attributes: { tagName: Tag } } ) { | ||
| return ( | ||
| <Tag className="wp-block-comments-query-loop"> | ||
| <InnerBlocks.Content /> | ||
| </Tag> | ||
| ); | ||
| }, |
There was a problem hiding this comment.
I added a deprecation with the old class name to bypass the validation error. I've read a few times the deprecation guide, but I still don't know if this is a use case that is supposed to be handled like this. In a sense, I added a save with the old class name to say "hey Gutenberg, this change is expected and the attributes didn't change, so just update the HTML and don't bother the user with a warning message".
There was a problem hiding this comment.
We might not need the deprecation (but can add the relevant logic to the Comments block's save.js instead?)
There was a problem hiding this comment.
add the relevant logic to the Comments block's save.js
What logic? (sorry I'm not following 😄)
| @@ -20,6 +20,7 @@ import { hasBlockSupport, getBlockDefaultClassName } from '@wordpress/blocks'; | |||
| * @return {Object} Filtered props applied to save element. | |||
| */ | |||
| export function addGeneratedClassName( extraProps, blockType ) { | |||
| if ( blockType.name === 'core/comments' ) return extraProps; | |||
There was a problem hiding this comment.
But the old save function added to the deprecated doesn't work yet by itself because this function (called in the blocks.getSaveContent.extraProps filter) is adding the wp-block-comments class name so we need to find a solution for that. This is just a quick fix to show that it validates properly if this is bypassed.
There was a problem hiding this comment.
called in the
blocks.getSaveContent.extraPropsfilter
Can we add our own filter to that hook in order to remove the old class name? 🤔
There was a problem hiding this comment.
Oooh, or maybe we can solve our issue using this filter?
There was a problem hiding this comment.
called in the
blocks.getSaveContent.extraPropsfilterCan we add our own filter to that hook in order to remove the old class name? 🤔
function setCommentsBlockCustomClassName( props ) {
let { className } = props;
const classes = className?.split( ' ' );
if ( classes.includes( 'wp-block-comments' ) ) {
className = classes
.filter( ( c ) => c !== 'wp-block-comments-query-loop' )
.join( ' ' );
}
return { ...props, className };
}
addFilter(
'blocks.getSaveContent.extraProps',
'core/comments/set-block-custom-class-name',
setCommentsBlockCustomClassName
);
Well, not quite there yet 😅
|
Thanks @luisherranz! A deprecation/migration could be one promising strategy. I'm still wondering, why are we getting the block invalidation error, when the legacy block conversion seems to be working for other blocks? I.e. what are they doing differently? (This is what lead me to assume that the problem here is that we have child blocks -- but maybe I'm mistaken?) |
|
I've figured out the problem and the reason was that in our case Gutenberg expects the 2022-04-27_18-33-23.mp4I think that this is not documented anywhere and is also very counter-intuitive. |
|
Well, the API version of the deprecated block is 2, so this makes sense. It's true that in this case, we would be using it to bypass those filters, but it still seems correct because it's explicit in the conditional that those filters shouldn't run for block types with API versions bigger than 1. I agree it'd be great to get confirmation from someone with more expertise, though 🙂 I guess another question is, if we bypass all those filters, what happens with the other functionality? For example, are now old blocks with custom class names triggering a validation error? Although it's possible that Gutenberg is dealing with that logic somewhere else because if not the same would happen to all the API > 1 blocks. |
|
To recap: The goal of this PR is to change the name (and ID) of the "Comments Query Loop" ( We've added logic to
The reason seems to be that we gutenberg/packages/block-library/src/comments/save.js Lines 6 to 12 in f297b61 We currently work around this by adding a deprecation (which also wasn't straight-forward; we had to set To me, it seem wrong that on top of the legacy block conversion logic, we also need a deprecation; I also think that it'll be easy to miss for devs that will work with this code in the future. I feel like that using I'd be curious to hear your thoughts @gziolo @youknowriad |
|
Hey @ockham to be honest, I'm not sure why you have to add a deprecation while with all the other previous case of legacyConvert, we didn't AFAIK. That said, this "legacyConvert" has always been a hack and not an official API, in that sense, it's possible that it doesn't work properly in all cases. I'm curious to try to understand why this block is different than the previous legacy conversion we did. |
Heh, that's exactly what puzzled me as well 😅 I can't say I've got a totally conclusive answer, but IIUC, the main difference is the What I think happens is that when the block is parsed for validation, the previous block class name (
Yeah, it did look like a possbile shortcoming to me... |
This is not a real fix. It's just a fast way to show where is the problem.
ockham
force-pushed
the
update/rename-comments-query-loop
branch
from
a8b9da2
to
ebcbe8d
Compare
|
Rebased to include #42081. |
|
This should be ready for another look. (I've updated the Testing Instructions in the PR description to cover all the weird little edge cases I ran into when testing this.) |
|
I've tested it, and it works as expected. 👍 |
|
As the PR is already approved, I'll merge it so I can rebase #41807 on top of these changes. |
bph
added
the
[Block] Comments
mburridge
added
the
Needs Dev Note
|
Added the Needs Dev Note label in case this needs a dev note (either individual or as part of a "misc" dev note) for WP 6.1 release. |
Left as a draft to have some feedback about the rename first, the options we have are:
What?
Renaming
Comments Query LoopwithCommentsWhy?
Right now, Comments Query Loop is not working like a Query block, it has not any option and is just rendering the comments, being able to add styling to them.
How?
Search and replace!
Testing Instructions
trunk, edit the "Single" template in the Site Editor to add the "Comments" block.wp:comments-query-loop. Copy-paste the block attributes, and the outermost<div />tag (including its HTML attributes) to a text editor of your choice; we'll need them later.update/rename-comments-query-loop).wp:commentsthere ✅ Compare the block attributes and the outermost<div />tag to the ones you copied earlier. They should be identical, except now there should be an additionalwp-block-comments-query-loopclass (for backwards compatibility).wp-block-comments-query-loopclass.Finally, try the same in the Post Editor.
Screenshots or screencast