-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
TT2: Add Comments block pattern #3136
base: trunk
Are you sure you want to change the base?
Conversation
Next, the pattern need to replace the block in the HTML templates. |
I've been trying inserting the pattern manually, and I'm getting the "Post Comments" block (rather than "Comments"), even with GB enabled. I've also tried changing the criterion to look for So it seems like our check isn't returning the desired (and required result). I'll poke a bit more to find out what's up. |
I looked a bit more into it; I've now also tried I think the reason is that the updated Comments block (and thus, I'll continue to look into this tomorrow. A few possible solutions off the top of my head:
|
Just for completeness, even once the issue with check for the existence of the Comments block is fixed, we need to do a bit more work: The Comments block has H2 for the "One reply to..." heading (via the Comments Title block -- which can actually be customized to different header sizes, but defaults to H2). However, it uses the Post Comments Form block for rendering the comments form, which is a thin wrapper around While that can be customized... comment_form(
array(
'title_reply_before' => '<h2 id="reply-title" class="comment-reply-title">',
'title_reply_after' => '</h2>'
)
); ...I'm not sure how to best integrate this fairly fine-grained level customization into the Comments -> Post Comments Form block hierarchy: Are we okay with just hard-wiring it as H2 (even though it used to be H3)? (This could affect existing themes.) Do we need to expose it in the UI? The latter might be the best option, but given that we're still using the "monolithic" Post Comments Form block -- which currently doesn't really have any customization options -- it might be a bit weird to expose only this one customization option. It might also look counter-intuitive to have a heading customization button in the block toolbar of a block that contains more than just a heading. These changes would need to be done in Gutenberg. I'll add a TODO item to the PR description, and I guess I'll file a separate issue in the GB repo to solicit design feedback. |
And finally, for the case where we the Comments block is unavailable and we have to use the Post Comments block, we need to make sure that that block renders an H2 for both "One reply to..." and "Leave a reply". This basically means applying this patch after all -- I think that's pretty much unavoidable; the block pattern technique that we're applying in this PR only helps if the Comments block is available. (Unless I'm missing something -- cc/ @carolinan) I'll add that also to the PR desc PR and will file a separate PR for it, as it can arguably be decoupled from what we're doing here. |
BTW unrelated to all the other prerequisites, there might be a slightly different way to implement the "block fallback": Instead of having a block pattern that renders either Comments or Post Comments, we could try something like including a fake |
That would break the users website when they switch themes, since that block would no longer be available. |
I think it needs to check for the WordPress version, not (only) if the block exists. |
src/wp-content/themes/twentytwentytwo/inc/patterns/comments.php
Outdated
Show resolved
Hide resolved
@@ -132,4 +133,4 @@ function twentytwentytwo_register_block_patterns() { | |||
); | |||
} | |||
} | |||
add_action( 'init', 'twentytwentytwo_register_block_patterns', 9 ); | |||
add_action( 'init', 'twentytwentytwo_register_block_patterns', 21 ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how I got the check for the new comments block to work on my setup running 6.0 + the plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great find! 🕵️♀️ Though I'm a bit hesitant to change the priority of this action; there might be a reason it was set to 9
(which I think is one before the default 10
) 🤔
But we could maybe add the comments
pattern in a separate action
that we add with priority 21
(just to play it safe) 🤔
I tried to reproduce this with 5.9 and installing/uninstalling the plugin and I don't see what you describe. With or without the plugin the comments show correctly with either version of the block both in the frontend and the editor. |
@carolinan what do you think this PR needs doing at the moment? I've tested on 6.0 and 5.9 with and without the plugin and the comments show as expected using the correct block for the case. |
I might be able to answer part of that; the original motivation for this PR was accessibility (see WordPress/gutenberg#43203). To fully address that, we might need some additional changes (e.g. #3136 (comment)). However, in order to improve UX (especially to avoid the -- arguably confusing -- "Please update your block to its editable mode" message), we might consider landing this PR as-is, and address a11y separately.
Thank you for testing @MaggieCabrera! This is the part I never seemed to have gotten to work properly. I'll retest myself now. |
fe37ce0
to
dbcaa4b
Compare
We have a few more occurrences of the Post Comments block that we'll need to replace in TT2: wordpress-develop/src/wp-content/themes/twentytwentytwo/templates/single-no-separators.html Line 31 in 83421f4
wordpress-develop/src/wp-content/themes/twentytwentytwo/templates/page-no-separators.html Line 14 in 83421f4
Note that two of those have block attributes. Not sure if that will work with our block-pattern approach 😕 |
Furthermore, we might want to add styling for wordpress-develop/src/wp-content/themes/twentytwentytwo/theme.json Lines 246 to 252 in 60e13f4
|
Yeah, that makes sense 🤔 😕 This means we'll be limiting use of the Comments block in TT2 a bit more (i.e. even when it's present because GB is installed), but it's probably worth avoiding the kind of situation that you described...
That's curious 🤔 What I could imagine is that if the template is unmodified, it will retain the pattern and thus the check. If it evaluates it on the frontend, it should render the correct block. However, the moment we modify the template that includes the comments pattern, I'd expect the situation @carolinan was describing if we then disable the Gutenberg plugin on an older WP install... |
Yeah, in the same way that adding any kind of other block and then uninstalling the plugin that adds that kind of block would render it invalid. Should we account for that kind of usage? Resetting the template would bring the old block back again in such a case. |
Good point. I'm not sure TBH -- I can't seem to think of any established precedents here. We could consider starting with the WP version check since it's "tighter" (and avoids that situation altogether, while still providing better UX for anyone on WP >= 6.1), and loosening it later. @MaggieCabrera Would you have time to take it from here? I'm still kinda busy with backports and such 😅 |
Mmmh, I don't think we need to change anything. TT2 launched with the old block and no CSS related to it. When implementing the new block we are trying to imitate the style of the old block instead, so they should look as close as possible (I'm saying this without checking this change specifically). |
Yeah, I can take it from here. Also, @carolinan might be able to confirm but I don't think theme changes need to be restricted to the freeze date the same way Gutenberg features need to be, being themes and all, not core changes. |
I pushed a change to those templates, adding the attributes to the group block wrapping them. |
I have not been working so I have not been able to stay up to date with the changes. Requested change: |
@@ -17,9 +17,9 @@ | |||
|
|||
<!-- wp:post-content {"layout":{"inherit":true}} /--> | |||
|
|||
<!-- wp:group {"layout":{"inherit":true}} --> | |||
<!-- wp:group {"layout":{"inherit":true},"style":{"spacing":{"padding":{"top":"var(--wp--custom--spacing--medium, 6rem)"}}}} --> | |||
<div class="wp-block-group"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The spacing needs to be added to the element, not only in the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch!!!
} | ||
return array( | ||
'title' => __( 'Post Comments block', 'twentytwentytwo' ), | ||
'content' => '<!-- wp:heading --><h2>' . __( 'Comments', 'twentytwentytwo' ) . '</h2><!-- /wp:heading --><!-- wp:post-comments /-->', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I may have found an issue. This heading will display even if comments are closed and there are no comments, and this will be an unexpected side effect to users.
Is it possible to check if comments are enabled before outputting the heading?
It depends on if that information is available at the time (init) when the pattern is registered as we would need to check it per post /page. -I don't think it is available.
Even if we found a way, this would not update if the user saves the template, and then we would be back to not having a heading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how I like this heading at all. It wasn't there on the original theme, and we are adding it now because the post comments block has an h3 instead? What are we trying to solve, the hierarchy of headings on the page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, the incorrect heading order is an accessibility issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, but we've had the problem in the old block for a year and are only doing something about it now. I think the fix for that is to actually make use of the new block, and if you want to make sure that people running older versions get the fix too, then fix the block. I think this added heading is causing bigger problems than it really solves.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I hadn't read the whole conversation on the original issue, I'll think more about this problem, see how we can solve it. I'm not too happy that this is a TT2 only solution, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok that makes sense to me, thanks for the context! and about the problem with pages/posts with no comments, it's tricky cause we are deciding if the heading is going to be present when registering the pattern, and we don't have a post id to check against, since some posts will have comments and some will not, like you said. Does it make sense to keep the heading for posts without comments if we find a way to hide it accesibly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the heading can be used to add context if there is a comments form but no comments, or if there are comments and the comment form / comment submission is closed. But not if comments are closed and there are no comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the theme should include a comments.php file.
I also don't think the old block should be updated and that change backported to older versions... WP doesn't do that, right? Only for security updates?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the heading can be used to add context if there is a comments form but no comments, or if there are comments and the comment form / comment submission is closed. But not if comments are closed and there are no comments.
But we don't have the contest on pattern registration to know that, do you know how we could do that?
I don't think the theme should include a comments.php file.
You mean because of the name of the file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if we replace the h3 to an h2 using the render_block
hook?
Thanks a lot for handling this, @MaggieCabrera and @carolinan! I'm not sure what the outcome was with regard to whether this change can go in after Core Feature Freeze:
FWIW, if you can give it approval, we can try to get a Core committer to merge it for us prior to Feature Freeze.
My 2 ct: I think that's preferable, since the heading workaround did seem to have a few unwanted side-effects that we hadn't fully figured out yet. (I'll yield to y'all's decision however since I'm no longer actively working on this PR.) |
A recap of what this PR ended up doing: The Post Comments block uses an h3 heading on it, this causes a11y issues on the templates where it's inserted, since there are no existing h2 in them. The new Comments block doesn't have the same problem because it allows you to change the heading as you need, but it's only available with Gutenberg or WP >= 6.1. To solve this issue, we are doing the following:
To test this:
|
Sounds pretty good to me! |
Thank you! I'll use this to update the PR description. |
Thank you @MaggieCabrera ! |
@carolinan If you get a chance, would you mind giving this a review? I talked to a Core committer and they seemed to agree that this might qualify as an a11y bugfix, so we might still be able to get this into WP 6.1 😊 |
Pinging WP 6.2 Editor Tech co-leads @Mamaduka and @ntsekouras. This is a PR that I hoped to get into WP 6.1 but that didn't make the cut at the time. Flagging for y'all in case you would like to get it into WP 6.2 😊 |
9d168d2
to
34a5879
Compare
src/wp-content/themes/twentytwentytwo/inc/patterns/hidden-comments.php
Outdated
Show resolved
Hide resolved
src/wp-content/themes/twentytwentytwo/inc/patterns/hidden-comments.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added a commit to address a small docblock standards issue.
This PR looks good to me 👍
I'm currently working on committing various changes to bundled themes. I can handle this one if needed.
Thank you, @audrasjb! That would be great! |
Hi, are we considering this for 6.2.1 or 6.3? |
Thank you @audrasjb! I can land this within the next couple of days so it'll go into 6.3. I don't feel strongly about whether or not it should go into 6.2.1 -- curious if folks think it qualifies as important enough for that 😊 |
Co-authored-by: Carolina Nymark <[email protected]>
d9a0159
to
0ec64e3
Compare
I imagine since this doesn't relate to 6.2, we can't include it in 6.2.1. Let's aim for 6.3. |
While I'm nominally the PR author, all credit really goes to @MaggieCabrera for doing most of the work, and to @carolinan for the original suggestion.
The Post Comments block uses an h3 heading on it, this causes a11y issues [as discussed in #43203 (and previously https://core.trac.wordpress.org/ticket/55172 )] on the templates where it's inserted, since there are no existing h2 in them. The new Comments block doesn't have the same problem because it allows you to change the heading as you need, but it's only available with Gutenberg or WP >= 6.1. To solve this issue, we are doing the following:
render_block
hook to swap the h3 with an h2, and solve the a11y for themTo test this:
hidden-comments.php
, since this branch is still not a 6.1 version of WP:if ( WP_Block_Type_Registry::get_instance()->is_registered( 'core/comments' ) && version_compare( $GLOBALS['wp_version'], '6.1', '>=' ) ) {
Aside from a11y, note that this PR also improves User Experience of the TT2 theme: Users with WP >= 6.1 will get the Comments block in editable mode automatically, rather than seeing the legacy mode and the upgrade notice:
Related PRs
Trac ticket: https://core.trac.wordpress.org/ticket/55172
This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.