Skip to content

Conversation

@cbravobernal
Copy link
Contributor

@cbravobernal cbravobernal commented Apr 12, 2022

What?

Adding the comment id to each comment on the Comments Query Loop.

Why?

Feature parity with previous WordPress Comments markup before going to blocks. This WP Tavern post pointed out the issue with the new Comment Query Loop block:

The first issue I ran into was the missing comment ID in the front-end output. This is necessary for the user’s browser to jump back to their comment after submitting one via the form. I also suspect this is necessary for the comment-reply JavaScript to work when clicking the reply link.

That suspicion turns out to be true:

Before After
comment-before comment-after

Note how the comment form opens inline (in the position where the comment will show up, i.e. after the last child of the comment we're replying to). This is because of WordPress' comment-reply.js script, which is enqueued by the Post Comments Form block.

Note that this behavior is consistent with a lot of (non-FSE) themes prior to Twenty Twenty-Two. Twenty Twenty-Two on the other hand uses the (monolithic) Post Comments block which currently doesn't support this behavior; instead, the user always has to fill in the comment form at the bottom (below all other comments).

How?

Just adding the comment id on the PHP function that is rendering.

Concerns

There are a few problems with this:

  • The comments form -- when rendered inline -- has some quirks in terms of layout (which we might be able to fix). Done, see Add comment id to all comments inside comments query loop #40268 (comment)

  • The comments form cannot currently be customized through the Site Editor. (This includes the UI in a broader sense, plus styling as well as copy such as "Reply to..." or "Cancel reply".) This is arguably not ideal, since it goes against the grain of enabling the user to use modular comments blocks to customize the way that comments are rendered. It could be argued that it's a result of the Post Comments Form block still being monolithic, and that having it is a necessary evil for the time being.

  • comment-reply.js allows some customization (e.g. of those strings) through data- attributes on the form. However, since these cannot be directly accessed through the Site Editor, we're left with the default values. This is a bit of a problem in terms of microtypography/styling: Note in the "After" gif that there is no whitespace between "Reply to admin" and "Cancel reply". The reason for this is that by default, there is no space in between those; themes either style them to add some margin, or customize the strings to add a space.

  • Twenty TwentyTwo theme has an issue: "Cancel Reply" link is next to the username to reply to. This has been happening in all themes since Twenty Eleven, and it has been always solved by using CSS, which is an approach that we should not use in block themes. I've seen that this happens to most of Block themes on WordPress environment.
    Screenshot 2022-04-13 at 10 12 50
    cc: @WordPress/theme-team

While there's an implicit connection between the Comments Query Loop and Post Comments Form block (the reply link won't work if the Post Comments Form block isn't also present), this isn't really due to this change: Even on trunk, if the Post Comments Form block was absent, the Comments Query Loop block's Reply links wouldn't work.

It's however arguable that the enqueuing of comment-reply.js should maybe happen in Comments Query Loop, rather than Post Comments Form.

We need to decide if we're okay with these glitches, or if we need to fix them. If the latter, we need to find out if they can be (easily) fixed (in time for WP 6.0).

Testing Instructions

1.) Create a post with comments.
2.) Add Comment Query Loop.
3.) Add Post Comments Form.
3.) Check that ids are on the markup.
4.) Reply a comment and check that you end up in that reply after the page reload.

@cbravobernal cbravobernal force-pushed the update/add-comment-id-on-comments-loop branch from 69ed605 to 7a719d0 Compare April 12, 2022 13:40
@cbravobernal cbravobernal requested review from DAreRodz and ockham April 12, 2022 14:03
@cbravobernal cbravobernal marked this pull request as ready for review April 12, 2022 14:03
Copy link
Contributor

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

I see everything correct, although PHP Unit tests need to be updated.

See https://github.com/WordPress/gutenberg/runs/5991056665?check_suite_focus=true#step:8:45


/**
* Enqueue the comment-reply script.
* Line commented until we fix this behaviour causing a display issue.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there an open GH issue about this problem? It would be great to have it linked to this PR. 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to create it. Is not so easy to explain 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

(I've reverted this change for now, see updated PR description.)

@cbravobernal cbravobernal marked this pull request as draft April 12, 2022 16:32
@ockham ockham marked this pull request as ready for review April 12, 2022 20:30
@cbravobernal
Copy link
Contributor Author

I mentioned also the issue in #core-themes Slack channel. I will update it here if we get some feedback about it.

@cbravobernal cbravobernal added [Block] Comment Template Affects the Comment Template Block [Type] Bug An existing feature does not function as intended labels Apr 13, 2022
Copy link
Contributor

@DAreRodz DAreRodz left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👌

@jffng
Copy link
Contributor

jffng commented Apr 13, 2022

Thanks @c4rl0sbr4v0 !

It's however arguable that the enqueuing of comment-reply.js should maybe happen in Comments Query Loop, rather than Post Comments Form.

I may be missing something, but think enqueuing comment-reply.js in the Comments Query Loop makes sense, maybe there's a way to check if the site & post has comments open before enqueuing it?

I followed your test instructions on this PR — replacing TT2's Post Comments Form block in the single template with the Comments Query Loop. But when I click "reply" to a comment just reloads the page, but does not open a reply for me:

Screen Shot 2022-04-13 at 12 03 08 PM

Am I missing something?

@cbravobernal
Copy link
Contributor Author

I may be missing something, but think enqueuing comment-reply.js in the Comments Query Loop makes sense, maybe there's a way to check if the site & post has comments open before enqueuing it?

Yeah, that would not be so hard to do.

I followed your test instructions on this PR — replacing TT2's Post Comments Form block in the single template with the Comments Query Loop. But when I click "reply" to a comment just reloads the page, but does not open a reply for me:
Am I missing something?

I forgot to add to the test instructions that in order to see the reply JS interaction, you need to have both Post Comments Form and Comment Query Loop in the same post/page/template. 🙇

The block you have to replace is Post Comments 😅

@jffng
Copy link
Contributor

jffng commented Apr 13, 2022

Got it, that makes sense.

One thing I noticed aside from the styling issues — replying to a nested comment just shifts the position of the reply form, rather than it being directly below the comment you are replying to. Is that expected?

Kapture.2022-04-13.at.13.53.55.mp4

I can't speak to the site editor capabilities of the comments form, but I do think we should address the layout quirks to at least reach parity with the previous layout of TT2s comments.

@ockham
Copy link
Contributor

ockham commented Apr 13, 2022

I've pushed a commit that should fix most layout quirks.

The problem that we saw earlier was that WP has a frontend script called comment-reply.js that will move the comment form to the position of the 'Reply' link that the user clicked. That script assumes that the form is wrapped in a container element with ID respond.

The Post Comments Form block however wrapped that markup in an additional <div />, to which it added the block class (wp-block-post-comments-form) and other wrapper attributes. Note that the class name is used for some styling of the Post Comments Form block.

image

This means that the comment-reply.js script would take the respond element and move it to the desired location, but not the wrapping <div /> with all those block-specific attributes (and styling) 😱

The solution is to tweak the markup that the Post Comments Form produces such that the outermost <div /> serves both as the container element for the block (class="wp-block-post-comments-form ...") as well as for the form (id="respond" class="comment-respond ...").

image

(Changing the markup produced by the block means we might be breaking any existing styling done by 3rd parties, but that shouldn't be a problem in this case since the Post Comments Form block has been marked experimental up to now.)

@ockham
Copy link
Contributor

ockham commented Apr 13, 2022

One thing I noticed aside from the styling issues — replying to a nested comment just shifts the position of the reply form, rather than it being directly below the comment you are replying to. Is that expected?

Yes and no 😅 Some of it is due to the layout quirks that I hopefully just fixed.

However, what you're seeing is partly expected: The comment form isn't strictly speaking supposed to be

directly below the comment you are replying to

Instead, it's supposed to be in the position where the comment will show up after posting. This means that if you reply to a comment A that already has a reply B, the form will show up below that other reply B. And -- indentation aside -- this just happens to be the same location as when you reply to reply B (if B doesn't have any replies itself yet).

To illustrate: Start with top-level comments A and C, and a reply to A: B.

A
|-B
C

Now, reply to A:

A
|-B
|-Form
C

Reply to B:

A
|-B
  |-Form
C

@peterwilsoncc
Copy link
Contributor

Frequently enqueueing the comment-reply script is wrapped in a conditional to make sure it's needed

if ( is_singular() && comments_open() && get_option( 'thread_comments' ) ) {
  wp_enqueue_script( 'comment-reply' );
}

You may be able to do a simpler check for the option if:

  • the comment form is designed to work on index pages too (but check the script can handle that)
  • the comments_open() check has already take place before displaying the form

@jffng
Copy link
Contributor

jffng commented Apr 14, 2022

I've pushed a commit that should fix most layout quirks.

This is looking much better, thanks @ockham !

Screen Shot 2022-04-14 at 9 48 21 AM

One thing is the "Cancel Reply" butts up right next to the author name, is there a way to introduce some padding there?

@ockham
Copy link
Contributor

ockham commented Apr 14, 2022

Thanks for testing again, @jffng!

One thing is the "Cancel Reply" butts up right next to the author name, is there a way to introduce some padding there?

Yeah, we're looking into that! (We might tackle that in a separate PR in order to unblock some other work 😊)

@ockham ockham added the Backport to WP 6.9 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 14, 2022
@ockham ockham added this to the WordPress 6.0 milestone Apr 14, 2022
@ockham ockham force-pushed the update/add-comment-id-on-comments-loop branch from b22119e to bbe09a4 Compare April 14, 2022 14:46
@ockham ockham force-pushed the update/add-comment-id-on-comments-loop branch from bbe09a4 to c73e14e Compare April 14, 2022 14:59
@ockham
Copy link
Contributor

ockham commented Apr 14, 2022

@peterwilsoncc Thanks a lot for your suggestion! Since we're currently only enqueuing comment-reply.js from the Post Comments Form block (and not touching it in this PR), I'm thinking to address this in a separate PR (where we might enqueue it from other blocks, too).

@ockham ockham merged commit a3f4d69 into trunk Apr 14, 2022
@ockham ockham deleted the update/add-comment-id-on-comments-loop branch April 14, 2022 17:38
@ndiego ndiego removed this from the WordPress 6.0 milestone Apr 19, 2022
adamziel pushed a commit that referenced this pull request Apr 19, 2022
For feature parity with previous WordPress Comments markup (prior to FSE). [This WP Tavern post](https://wptavern.com/new-comment-related-blocks-arriving-with-wordpress-6-0#:~:text=The%20first%20issue%20I%20ran%20into%20was%20the%20missing%20comment%20ID%20in%20the%20front%2Dend%20output.) pointed out the issue with the new Comment Query Loop block:

> The first issue I ran into was the missing comment ID in the front-end output. This is necessary for the user’s browser to jump back to their comment after submitting one via the form. I also suspect this is necessary for the comment-reply JavaScript to work when clicking the reply link.

Co-authored-by: Bernie Reiter <[email protected]>
@adamziel
Copy link
Contributor

This was cherry-picked to wp/6.0 branch but still needs a wordpress-develop backport cc @gziolo

@gziolo
Copy link
Member

gziolo commented Apr 19, 2022

I see the same changes in WordPress/wordpress-develop#2603 👍🏻

@ockham ockham mentioned this pull request Apr 20, 2022
13 tasks
@adamziel adamziel removed the Backport to WP 6.9 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Apr 25, 2022
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] Bug An existing feature does not function as intended

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

9 participants