-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Add placeholder to Post Comments block #40484
Conversation
Size Change: -547 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
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.
Nice job, I think it looks great already! 👍 I only have some minor comments here
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.
It looks great to me! 🙂
Would it make sense to add some css to prevent interactivity in the editor? I mean, as the links don't work and the textarea
, for example, doesn't accept input, maybe we could prevent the clicks there with something like:
.wp-block-post-comments * {
pointer-events: none;
}
Thanks, Mario. I'll add that CSS 🙂 |
Maybe adding |
For me, this looks great so far. Thaks for taking care of this, @luisherranz! 🙌 PS: it's a bit weird to see the warning message styles changing when you modify, e.g., the comments color or font size. 😅 Not sure if we could/should do something regarding that. |
Yes, that should be definitely fixed, but probably out of the scope of this PR. EDIT: I've opened a separate issue for that #40536.
EDIT: Removed in favor of I saw in @michalczaplinski's PR (#40368) a bit of logic to show different warning messages if the post you're editing doesn't support comments for some reason, so I've added it here as well. It took it a bit further, though. @michalczaplinski could you please check it out, let me know what you think, and maybe if you like the approach, merge this and reuse the same logic for your block (create a hook that can be shared, or something like that). This is the flow I've used: And this is the video explanation: |
Thanks a lot @luisherranz, this is looking great! @priethor has asked over at #40368 for some design feedback on how to make it even clearer that these placeholders aren't really editable, to which @jasmussen kindly replied. Would it be possible to apply Joen's suggestions to this PR as well? 😊 🙏 |
I'm on it 🙂 |
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.
LGTM! But still have some doubts if how accessible is this block for screen readers.
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.
Thanks @luisherranz! This is looking great 👍
Let's merge this as-is.
Some ideas for follow-ups:
- We might be able to figure out the CSS quirks you mentioned in your Loom in a follow-up.
- I also don't know how this is going to work in terms of a11y. Maybe we can have someone who has a better grasp of a11y review this, so we can fix it in a follow-up.
- We'll go with the Warning for now (even though this is likely not an established UX pattern for this). However, with Post Comments block: Merge Comments Query Loop block #40521, the placeholder will only be used for an empty Post Comments block, so we'll change the warning there to reflect that.
- Maybe we can re-use the Comment Form block placeholder (Post Comments Form: Add "proper" visual representation in the editor #40368) to render the form here (and de-dupe code), once that's merged.
- We could consider making the placeholder resemble the Comments Query Loop a bit more -- specifically e.g. by using the post's actual comments when inside the Post Editor.
👋 @ryelle If you can spare some time, could we have your a11y feedback on this PR? It basically adds a placeholder for a post's comments (and comment form) to the editor which cannot really be edited -- really just to have something that somewhat captures what it'll look like on the frontend. We were a bit unsure if this will surface to screenreaders in an undesired way 😅 You need this patch to re-enable the (currently deprecated) Post Comments block: diff --git a/packages/block-library/src/post-comments/block.json b/packages/block-library/src/post-comments/block.json
index 6c5c4e16b8..8a15e57569 100644
--- a/packages/block-library/src/post-comments/block.json
+++ b/packages/block-library/src/post-comments/block.json
@@ -33,8 +33,7 @@
"background": true,
"text": true
}
- },
- "inserter": false
+ }
},
"style": [
"wp-block-post-comments", Then, insert the Post Comments block in the Post or Site editor to see the placeholder. |
(Merging as discussed above; additional feedback to be addressed in follow-up PRs.) |
@ockham thanks for the ping 😄 I did some quick testing - I hope this helps. When I turned on VoiceOver and looked at the different ways to navigate through the page, the content in the Post Comment block shows up. Like in the heading list, the “One response…” and “Leave a Reply” are there, when other headings in the page aren't (because heading blocks have different roles). I think this might be a larger gutenberg issue though, and I don't know what the expected behavior should be. When the block is added, the first thing I hear is the warning, and then it’s possible to navigate through the whole comment section. I think this is good, since the user would want to know what’s in there. When navigating by keyboard or VoiceOver + keyboard, it’s possible to focus on the links, and activating a link reloads the page (pretty sure the root issue is this #15799). The comment textarea and button are disabled, as expected. The textarea has no label, not sure if that’s from the theme or something to fix in the block. I was able to change the styles with keyboard & VoiceOver, but tweaking the typography settings (like letter spacing) also changed the warning. Not an a11y issue, just unexpected 🙂 |
Thanks a lot for the review, @ryelle!
That's actually my fault. The label is there, but I removed the textarea id because the linter was complaining. I'll fix it in a new PR. Thanks for the heads up! EDIT: PR created #40527
It seems like it, yes.
Yes, @DAreRodz also mentioned it here #40484 (comment). I haven't found any issue that describes that problem, so I'll open one. EDIT: Issue created #40536.
Do you know about any other block that contains similar headers, but they don't appear? (to take a look at the code and see if they are doing something different). |
Thanks a lot for your a11y feedback, @ryelle -- much appreciated! |
Cherry-picked to the |
Backport of the exposure of the |
This is a backport that exposes the default_comments_status option in the Editor, under the discussion settings, which then is then used by the Post Comments and Post Comments Form blocks. Related change in Gutenberg: WordPress/gutenberg#40484. Props luisherranz. See #55567. git-svn-id: https://develop.svn.wordpress.org/trunk@53259 602fd350-edb4-49c9-b593-d223f7449a82
This is a backport that exposes the default_comments_status option in the Editor, under the discussion settings, which then is then used by the Post Comments and Post Comments Form blocks. Related change in Gutenberg: WordPress/gutenberg#40484. Props luisherranz. See #55567. Built from https://develop.svn.wordpress.org/trunk@53259 git-svn-id: http://core.svn.wordpress.org/trunk@52848 1a063a9b-81f0-0310-95a4-ce76da25c4cd
This is a backport that exposes the default_comments_status option in the Editor, under the discussion settings, which then is then used by the Post Comments and Post Comments Form blocks. Related change in Gutenberg: WordPress/gutenberg#40484. Props luisherranz. See #55567. Built from https://develop.svn.wordpress.org/trunk@53259 git-svn-id: https://core.svn.wordpress.org/trunk@52848 1a063a9b-81f0-0310-95a4-ce76da25c4cd
What?
This PR adds a placeholder in the Editor for the Post Comments block. It includes:
A warning at the top with the text:
The navigation links
A comment example, including an avatar, author name, date, content, and reply link.
The reply form.
Why?
Before this, the Post Comments blocked rendered:
Post comments block: no post found.
No comments
.None of those are especially useful, considering the only action users can perform in this block is to configure its style.
How?
I've added the default comments markup. The only dynamic parts are the post title and the user name (to simulate the reply form message).
I've used Gutenberg's
<Warning>
component for the warning. I'm not a big fan of how it looks like in the UI, but I think it's out of the scope of this PR to address that.I acknowledge that this type of placeholder is far from ideal, but @DAreRodz and I analyzed the other possibilities, and we concluded that it was the least bad option. The other possibilities we considered were:
Using
<ServerSideRender>
component.Executing
comment_template()
didn't work within a REST API call (/wp/v2/block-renderer/
) because it doesn't have a post context.Even if that could be solved, the result wouldn't faithfully represent the frontend because:
comment_template()
function.Looping over the real comments in the Editor to reproduce the final HTML as faithfully as possible.
Replicating the logic of the comments is not trivial (as we've seen in the Comments Query Loop blocks).
Also, the result wouldn't faithfully represent the frontend because:
So at the end, and considering that this is a temporary block that should be removed once the Comments Query Loop and the Comments Form blocks are mature enough, we decided to add the default comments markup so the user can get a notion of the styles, and a warning to prevent confusion.
Testing Instructions
Site Editor:
Post Editor:
Screenshots or screencast
https://www.loom.com/share/91b518264de84e75a7c3a64e283ee0c2