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

Block Library: Add new Comment Reply Link block #35774

Merged
merged 11 commits into from
Oct 27, 2021

Conversation

DAreRodz
Copy link
Contributor

@DAreRodz DAreRodz commented Oct 19, 2021

Description

This PR implements a block which adds a link to reply to a specific comment. The implementation will follow the design explained in the linked issue.

Will close #30576.

How has this been tested?

Typography, align and Color settings
  1. Add a post comment block (using an existing comment ID).
  2. Add an inner post comment reply link block.
  3. Confirm that the color and typography options work.
  4. Confirm that the text align option works.

NOTE: there is an issue related to global styles, check #35774 (comment)

Threads depth limit
  1. Create a comment (comment 1).
  2. Create another comment, replying the previous one (comment 2).
  3. Go to site editor and, in the Single Post template, add a post comment block with the reply link for each comment.
  4. In the frontend, go to a post and confirm that both render the correct reply link.
  5. Go to Discussions Settings and change the "levels deep" setting to 2.
  6. Confirm that only comment 1 renders a reply link.
  7. Go back to Discussions Settings and disable threaded comments entirely.
  8. Confirm that none of the comments render a reply link.
Invalid or missing comment ID
  1. Add a comment block and set an invalid comment ID.
  2. In the frontend, confirm that the block is not rendered.
Only registered and logged in users can comment
  1. In Discussions Settings, enable "Users must be registered and logged in to comment"
  2. Add a comment block in the Single Post template, and set a valid comment ID.
  3. Visit a post in incognito.
  4. confirm that the rendered reply link shows a Log in to reply message or similar.

Types of changes

New feature

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 github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Oct 19, 2021
@github-actions
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @DAreRodz! 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.

@gziolo gziolo added [Block] Comment Template Affects the Comment Template Block New Block Suggestion for a new block labels Oct 21, 2021
@DAreRodz DAreRodz force-pushed the add/comment-reply-link branch 3 times, most recently from 8c6acc9 to dd4c4a8 Compare October 25, 2021 16:20
@DAreRodz
Copy link
Contributor Author

Hey! I want to explain what I did so far. It would be great to receive some feedback here. 😊

To generate the reply link, I'm using the get_comment_reply_link() function that exists on WordPress. That function already covers by default some of the cases mentioned by @SantosGuillamot in the issue, like changing the text when a user is not logged in (if only users can comment), or hiding the link when comments are disabled.

To hide the reply link when the comment has a certain depth, I've created a loop that iterates over all the comment's ancestors. Is this loop OK, or is there a better way to know the depth value of a comment?

https://github.com/WordPress/gutenberg/blob/55594dac09ba91b36fbda32fbe70bf0b3a9d84cb/packages/block-library/src/post-comment-reply-link/index.php#L26-L36

And also, related to that: should the reply link be hidden in the editor as well when reached a certain depth? In that case, I guess the same loop should be replicated in the edit.js file to compute the depth value.

cc: @gziolo @ntsekouras

width="24"
height="24"
viewBox="0 0 24 24"
fill="none"
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've just copied the icon from the design, and seems that this fill attribute is overwritten by

.block-editor-block-icon.has-colors svg {
    fill: currentColor;
}

making the icon look like this:

image

@jameskoster, do you know if there is a quick way to fix this or does the svg need to be changed?

Copy link
Contributor

Choose a reason for hiding this comment

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

That looks like the wrong svg code anyway, let me grab the correct one:

<svg width="24" height="24" viewBox="0 0 24 24" xmlns="http://www.w3.org/2000/svg">
<path d="M6.68822 10.625L6.24878 11.0649L5.5 11.8145L5.5 5.5L12.5 5.5V8L14 6.5V5C14 4.44772 13.5523 4 13 4H5C4.44772 4 4 4.44771 4 5V13.5247C4 13.8173 4.16123 14.086 4.41935 14.2237C4.72711 14.3878 5.10601 14.3313 5.35252 14.0845L7.31 12.125H8.375L9.875 10.625H7.31H6.68822ZM14.5605 10.4983L11.6701 13.75H16.9975C17.9963 13.75 18.7796 14.1104 19.3553 14.7048C19.9095 15.2771 20.2299 16.0224 20.4224 16.7443C20.7645 18.0276 20.7543 19.4618 20.7487 20.2544C20.7481 20.345 20.7475 20.4272 20.7475 20.4999L19.2475 20.5001C19.2475 20.4191 19.248 20.3319 19.2484 20.2394V20.2394C19.2526 19.4274 19.259 18.2035 18.973 17.1307C18.8156 16.5401 18.586 16.0666 18.2778 15.7483C17.9909 15.4521 17.5991 15.25 16.9975 15.25H11.8106L14.5303 17.9697L13.4696 19.0303L8.96956 14.5303L13.4394 9.50171L14.5605 10.4983Z" />
</svg>

I assume we'll add this to the icon library as a follow up?

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've updated the icon with the code you shared. Now it works.

image

I assume we'll add this to the icon library as a follow up?

Already added to the icon library (not sure if that's what you mean) 😄

@gziolo gziolo marked this pull request as ready for review October 26, 2021 12:37
@gziolo
Copy link
Member

gziolo commented Oct 26, 2021

I found two issues related to (Global) Styles that don't seem related to this PR.

Background color + link color trigger contrast checker warning when they shouldn't:

Screen Shot 2021-10-26 at 16 58 49

I think the checker compares the background color selected and the default text color read from the same div, but we don't care about it at all.

The selected font family has class names applied correctly, but it isn't reflected in the computer font family in the editor:
Screen Shot 2021-10-26 at 17 14 43

It seems to be correct on the frontend though:

Screen Shot 2021-10-26 at 17 18 27

From my perspective, the only blockers are those related to PHP notices and most importantly the one that produces the fatal error.

@DAreRodz
Copy link
Contributor Author

DAreRodz commented Oct 26, 2021

Thanks for your review, @gziolo. I've fixed all the issues you found. (*)
BTW, I was wondering if it is worth adding PHP tests for the SSR of this block ― something similar to /phpunit/class-block-library-navigation-link-test.php 🤔

(*) EDIT: all except those related to global styles.

- It uses `get_comment_reply_link` to generate the link.
- That function already cover most of the cases, except comments depth.
Using `#comment-reply-pseudo-link` for consistency.
That pattern is being used in other blocks, e.g.
Post Comment Author or Post Comment Date
The cases are:
- The comment ID passed is invalid
- The comment doesn't have any parents
- There is no link to render
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.

Nice work on this PR 👍🏻

It still needs a rebase after I landed the Comment Author Avatar block that changes whitespaces in the lib/blocks.php file.

BTW, I was wondering if it is worth adding PHP tests for the SSR of this block ― something similar to /phpunit/class-block-library-navigation-link-test.php 🤔

The Navigation block is far more complex. We could add PHP tests for the Comments Query Loop block once it is fully functional that tests the render_callback behavior. Although, I feel like e2e tests are simpler to write and give more confidence because they ensure that the user experience is also good. We can look at that after we have all the required blocks in place.

@gziolo gziolo added the [Package] Block library /packages/block-library label Oct 27, 2021
@gziolo gziolo changed the title Post Comment: Add Comment Reply Link block Block Library: Add new Comment Reply Link block Oct 27, 2021
@gziolo gziolo merged commit fb42d94 into WordPress:trunk Oct 27, 2021
@github-actions github-actions bot added this to the Gutenberg 11.9 milestone Oct 27, 2021
@andrewserong andrewserong added the [Type] Feature New feature to highlight in changelogs. label Nov 4, 2021
@gziolo
Copy link
Member

gziolo commented Dec 21, 2021

I tested again the issues raised earlier and it looks like the font family handling is no longer an issue:

The selected font family has class names applied correctly, but it isn't reflected in the computer font family in the editor:

This is how it looks like now:

Screenshot 2021-12-21 at 09 58 16

The issue with the color contrast is still present and I'm about to file a new issue for that to discuss possible solutions.

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 First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository New Block Suggestion for a new block [Package] Block library /packages/block-library [Type] Feature New feature to highlight in changelogs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Comment Reply Link Block
4 participants