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

Add edit post comment block #30671

Closed
wants to merge 9 commits into from
Closed

Conversation

diedexx
Copy link
Contributor

@diedexx diedexx commented Apr 9, 2021

Description

Fixes #30577
Adds a Edit Comment Link block which can be used inside a Post Comment block which renders the edit link if the logged in user has the right capabilities to edit the comment.

How has this been tested?

I've placed a Post Comment block on a post and selected an id of an existing comment. In that block, I've placed the new Edit Comment block and modified all available properties in the sidebar and checked if these changes were reflected on the published post.

Screenshots

image

Types of changes

New feature - New block type: Edit Comment.

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 Apr 9, 2021
@github-actions
Copy link

github-actions bot commented Apr 9, 2021

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

@carolinan
Copy link
Contributor

I wonder if the block name should be Post comment edit link,

I know that the pseudo link is used in more than one block, but I think this needs to be reconsidered because it is still possible to right click and open the link, or select and open the link using a screen reader, and the result is not the expected since the current page is opened instead.
But this needs to be part of a larger review of how to handle links in the editors.

I used the following markup to test the block in the post editor:

<!-- wp:post-comment {"commentId":place the id here} -->
<div class="wp-block-post-comment"><!-- wp:post-comment-edit /--></div>
<!-- /wp:post-comment -->

I only tested it with the administrator user role. Except for the remark about the link, the block is working well for me.
The fixtures needs to be added:
https://github.com/WordPress/gutenberg/blob/trunk/packages/e2e-tests/fixtures/blocks/README.md#creating-fixtures

Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

Thought I'd do a quick review and test, but I don't work regularly on FSE, so may be missing some aspects in my review.

Seems really good so far, nice work.

packages/block-library/src/post-comment-edit/edit.js Outdated Show resolved Hide resolved
packages/block-library/src/post-comment-edit/index.php Outdated Show resolved Hide resolved
</PanelBody>
</InspectorControls>
<div { ...useBlockProps() }>
<a href="#edit-comment-pseudo-link">{ __( 'Edit' ) }</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

Clicking the link in the editor seems to cause an unusual crash and some console errors, not sure what's happening, but it's probably unrelated to this PR. Thought I'd flag it though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice catch! It could cause some pretty nasty behaviour. I've created an issue for that: #30967

I wouldn't know how to adres this in the scope of this block though besides removing the href. But removing the href has its own issues.

@talldan
Copy link
Contributor

talldan commented Apr 12, 2021

I wonder if the block name should be Post comment edit link,

That name sounds better to me too 👍

@carolinan carolinan added the New Block Suggestion for a new block label Apr 19, 2021
@diedexx
Copy link
Contributor Author

diedexx commented Apr 19, 2021

Thank you for your feedback @carolinan and @talldan
I have applied most of your suggestions besides the fixtures, which I didn't get around to today.
I'll see if I can spare some time this or next week to add them, otherwise I'm more than fine with somebody else committing them.

@talldan
Copy link
Contributor

talldan commented May 6, 2021

@diedexx How's it going on this task? Let me know when it's ready for review or if there's anything I can help with.

@diedexx diedexx requested review from nerrad and ntwb as code owners May 7, 2021 13:06
@diedexx
Copy link
Contributor Author

diedexx commented May 7, 2021

Hi @talldan, I've just added the requested fixture. I'd really appreciate a review

@carolinan carolinan added the [Block] Comment Template Affects the Comment Template Block label Oct 8, 2021
Copy link
Contributor

@cbravobernal cbravobernal 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 done @diedexx!

I've suggested some changes in order to make this component similar to other existing ones like post-comment-author, post-comment-dateor site-logo.

Hope they are fine!

Comment on lines +15 to +23
<ToggleControl
label={ __( 'Open in new tab' ) }
checked={ openInNewTab }
onChange={ () =>
setAttributes( {
openInNewTab: ! openInNewTab,
} )
}
/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Using the same approach as site logo block could be:

https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/site-logo/edit.js#L278-L286

					<ToggleControl
								label={ __( 'Open in new tab' ) }
								onChange={ ( value ) =>
									setAttributes( {
										linkTarget: value ? '_blank' : '_self',
									} )
								}
								checked={ linkTarget === '_blank' }
							/>

Comment on lines +10 to +13
"openInNewTab": {
"type": "boolean",
"default": false
}
Copy link
Contributor

Choose a reason for hiding this comment

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

There are similar approaches already on production blocks. Maybe we could change this one in order to keep consistency between them!

For example:
https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/site-logo/block.json#L19-L22

Comment on lines +21 to +22
"fontSize": true,
"lineHeight": true
Copy link
Contributor

Choose a reason for hiding this comment

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

When I tested on my local site, it appeared a notice telling: fontSize support is now declared under supports.typography.fontSize

Using this new approach should solve it.

	"typography": {
			"fontSize": true,
			"lineHeight": true,
			"__experimentalFontFamily": true,
			"__experimentalFontWeight": true,
			"__experimentalFontStyle": true,
			"__experimentalTextTransform": true,
			"__experimentalLetterSpacing": true
		}

Comment on lines +27 to +29
if ( $open_in_new_tab ) {
$link_atts .= 'target="_blank"';
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If we change to the other blocks approach. We have to change this into something like attributes['linkTarget'] in order to work on the frontend.

export { metadata, name };

export const settings = {
title: _x( 'Post Comment Edit Link', 'block title' ),
Copy link
Member

Choose a reason for hiding this comment

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

The title and description should be in block.json so it gets exposed also on the server.

@gziolo
Copy link
Member

gziolo commented Oct 27, 2021

Let's close this PR in favor of #35965 opened by @c4rl0sbr4v0. It has additional iterations and is ready for testing. @diedexx, thank you for working on this PR, it was very helpful. All your comments were cherry-picked to the new branch 🎉

@gziolo gziolo closed this Oct 27, 2021
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Edit Comment Link Block
5 participants