Comment Content Block: Add typography, color and padding support #35183

DAreRodz · 2021-09-28T11:29:00Z

Description

This PR continues the implementation of the Post Comment Content block that already existed. It adds support for text alignment, font size, family, style and weight, line height, colors (including links and gradients), and vertical/horizontal padding.

Will close #30574.

How has this been tested?

Add a post comment block (using an existing comment ID).
Add an inner post comment content block.
Confirm that the color and typography options work.
Confirm that the text align option works.
Confirm that the padding option works.
Create a template containing just the Comment Content block, and confirm that the fallback message (The content of the comment.) is shown.

Types of changes

Enhancement

Checklist:

My code is (manually) 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 · 2021-09-28T11:29:25Z

👋 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.

mtias · 2021-09-28T13:43:30Z

Commenting here but we should transfer to the general tracking issue — the "Post" prefix is a bit pointless, we should simplify the naming of all these blocks to be "Comment Content", "Comment Author" etc.

ntsekouras · 2021-09-28T14:45:23Z

packages/block-library/src/post-comment-content/block.json

+			"lineHeight": true
+		},
+		"html": false,
+		"__experimentalLayout": true


I think we will not need to add the __experimentalLayout for this block. Layout is for container blocks that enables controlling width/height.

Thanks, @ntsekouras. I did that because @SantosGuillamot suggested adding that specific setting. I'll talk with him to clarify that.

I suggested adding that because it's something supported in the Post Content block, and I thought it could be a good idea to follow a similar pattern. It seemed to me that it would add more flexibility for editing the layout, but I don't have a strong opinion here, so no problem if you feel it's better to change it 🙂

@scruffian @MaggieCabrera @jffng curious what has come up in themes as uses here.

I've not seen a real life use case where we'd want to use this outside of a post comment block.

I think layout would make more sense in the parent blocks like Comment Loop

jameskoster · 2021-09-28T14:49:51Z

I understand that this happens when the comment ID is not specified in the parent block, am I right?

Correct. Example: editing the single post template in isolation, with no post data.

What should the block show when the comment ID is not correct? Is it OK showing the same message?

I presume you're referring to this interface in the current "Post Comment" block:

I don't think we should offer this functionality in the Comment Content block since it is strictly a template-only block, IE it will always have an ID passed to it by the queried post.

Regarding the icon, I guess I have to create a new one inside @wordpress/icons using the design, right?

Perfect! Here's the SVG:

<svg width="24" height="24" viewBox="0 0 24 24" fill="none" xmlns="http://www.w3.org/2000/svg">
<path fill-rule="evenodd" clip-rule="evenodd" d="M6.68822 16.625L5.5 17.8145L5.5 5.5L18.5 5.5L18.5 16.625L6.68822 16.625ZM7.31 18.125L19 18.125C19.5523 18.125 20 17.6773 20 17.125L20 5C20 4.44772 19.5523 4 19 4H5C4.44772 4 4 4.44772 4 5V19.5247C4 19.8173 4.16123 20.086 4.41935 20.2237C4.72711 20.3878 5.10601 20.3313 5.35252 20.0845L7.31 18.125ZM16 9.99997H8V8.49997H16V9.99997ZM8 14H13V12.5H8V14Z" fill="black"/>
</svg>

I'm not sure, but these settings don't seem quite possible to be added as that would modify the comment content, if I'm not mistaken.

Good point, these should probably be omitted :)

ntsekouras · 2021-09-28T14:57:13Z

I don't think we should offer this functionality in the Comment Content block since it is strictly a template-only block, IE it will always have an ID passed to it by the queried post.

I agree with @jameskoster , but let's change this when we have the comments loop block. What do you think?

jameskoster · 2021-09-28T15:17:26Z

~~Hmm, this is a new block, no? So no harm leaving it out?~~

~~To be clear, we should keep that interface for the Post Comment block, but not have it for the Comment Content block.~~

Edit: I understand now. Imo we should update the Post Comment block to check the ID when the user adds it, and throw an "comment not found" error (or similar) if they input a bad ID.

DAreRodz · 2021-09-29T09:45:11Z

Imo we should update the Post Comment block to check the ID when the user adds it, and throw an "comment not found" error (or similar) if they input a bad ID.

Makes completely sense, I'll do that. Thanks for all your feedback, guys. 🙌

packages/icons/src/library/comment-content.js

ntsekouras · 2021-09-30T10:38:02Z

packages/block-library/src/post-comment-content/edit.js

@@ -15,13 +24,23 @@ export default function Edit( { context: { commentId } } ) {
 		'content',
 		commentId
 	);
+
+	// Show a placeholder when there is no context.
+	if ( ! commentId ) {


If no commentId exists the message should be something like no comment exists.
If content is undefined has two cases:

still loading

comment doesn't exist (this can be handled in a separate PR though and has to do with useEntityProp)

The below if ( ! content?.rendered ) check (I had hastily added yesterday) is not correct as it should be shown when the comment exists and there is no content.

If no commentId exists the message should be something like no comment exists.

Maybe (or probably 😅) I'm wrong, but from what @jameskoster wrote in this comment, I understood that, if you don't have commentId defined, it could be one of these two cases:

You haven't defined commentId in the parent block (i.e., the Comment block). In that case, I assumed that the block responsible for showing a message like Input the comment ID would be the Comment block, and the Comment Content block wouldn't be rendered.

You are editing a template / template part / etc. that would be rendered inside a Comment block. For this one, I thought you would see a generic message like The content of the comment.

If content is undefined has two cases:

still loading

comment doesn't exist (this can be handled in a separate PR though and has to do with useEntityProp)

I see. Is there a way to know if an entity is still loading or if it doesn't exist after calling useEntityProp? I mean, like a value or a prop returned by that hook? That would be useful. 🙂

The below if ( ! content?.rendered ) check (I had hastily added yesterday) is not correct, as it should be shown when the comment exists and there is no content.

You mean that, if content is not defined, it's not that the comment has no content but that it doesn't exist, right? So I guess I should write something like if ( content && ! content.rendered ).

I guess we could have a placeholder chip like here when no commentId is provided.

You mean that, if content is not defined, it's not that the comment has no content but that it doesn't exist,

Correct.

Probably there is a couple of things we could do for this first version, where we do not have the comment content editable - also not sure if we need to..

Skip the check/placeholder for a comment that exists and has no content

Instead of useEntityProp, use the core/data selectors like:

const { comment, isLoading } = useSelect( ( select ) => { const { getEntityRecord, hasFinishedResolution } = select( coreStore ); const queryArgs = [ 'root', 'comment', commentId ]; return { comment: getEntityRecord( ...queryArgs ), isLoading: ! hasFinishedResolution( 'getEntityRecord', queryArgs ), }; }, [ commentId ] );

ntsekouras · 2021-10-04T10:14:19Z

Sorry for the temp close - bad gh cli command 😄

DAreRodz · 2021-10-05T16:14:38Z

Hey there, I've just created a simple schema in order to organize things better in my mind. The following diagram shows how (IMHO) the Comment Content block should behave in the context of the Comment block.

Excalidraw link

While doing so, I came up with some questions:

Maybe we can use the selector you proposed (Comment Content Block: Add typography, color and padding support #35183 (comment)) in the parent block instead, and handle there if we should render a loading/warning message when the entity is not ready or does not exist, skipping the children blocks. Would that make sense?
If so, should those changes be done in a different PR?
Am I overthinking here? Please, let me know. 😅

Apart from that, I've also been thinking about when the Comment block is rendered inside the Comment List block. In that case, I guess the Comment block should know whether it's being rendered inside or outside a Comment List. Is there a way for doing so, or would it need to be two different blocks?

Excalidraw link

DAreRodz · 2021-10-06T10:01:56Z

I've done the changes that @ntsekouras suggested in #35183 (comment). About the placeholder chip, I guess I would need some help here from the designers (cc: @jameskoster ). 😊

Also, if anyone can take a look at what I've posted right before, that would be great. 👍

DAreRodz · 2021-10-06T10:21:06Z

packages/block-library/src/post-comment-content/edit.js

+		return (
+			<div { ...blockProps }>
+				<Warning>
+					{ __( 'The specified comment does not exist.' ) }


Should I change the warning message to include the block name? Just to make it more meaningful.

Suggested change

{ __( 'The specified comment does not exist.' ) }

{ __( 'Comment Content: The specified comment does not exist.' ) }

jameskoster · 2021-10-06T13:57:33Z

Hmm, I'm a little confused as to why we need this state:

The UX is particularly poor because if I choose a bad ID when inserting the Post Comment block then I need to manually remove everything to start over.

Shouldn't we run the ID validation on this save button instead?

If this "The specified comment does not exist." state is just an interim until we can do that, then I suppose it's ok. But ultimately I don't think a user should ever actually see this? 🤔

mtias · 2021-10-06T14:48:26Z

A comment could be deleted, or the ID could be modified using the html editor, or coming from a different template, WordPress import, etc.

jameskoster · 2021-10-06T15:23:42Z

Makes sense. In that case I think it's ok for now, but we might want to revisit holistically later.

Just to clarify @DAreRodz, this is the "placeholder chip" you were referring to, correct? :D

DAreRodz · 2021-10-06T16:55:21Z

Thanks for your feedback! 🙌

@jameskoster I agree with you: the ID validation should run on the save button of the Comment block (actually, it's more or less what I posted in the previous diagram 😅). I can implement it (if that's okay with you), although I'm not sure if it's outside the scope of this PR. 🙃

A comment could be deleted, or the ID could be modified using the html editor, or coming from a different template, WordPress import, etc.

@mtias good point, I didn't think of that. Wondering if it would make sense to show a similar message in the SSR from PHP. 🤔

Just to clarify @DAreRodz, this is the "placeholder chip" you were referring to, correct? :D

Nope, I was referring to this one I've added to show something while the comment is being loaded, as suggested in #35183 (comment), but he was proposing to use it when commentId is undefined. I've just realized I misunderstood what Tsekouras was proposing ― sorry about that. 🙈 I think he meant to use the "placeholder chip" here instead.

jameskoster · 2021-10-06T17:35:56Z

although I'm not sure if it's outside the scope of this PR.

Yeah let's do it separately.

I think he meant to use the "placeholder chip" here instead.

Ah, the no-context placeholder :D

There's an ongoing initiative to simplify the styling of these. The Post Content block for example just outputs a paragraph now. I think we can do the same here.

mtias · 2021-10-07T12:25:08Z

Wondering if it would make sense to show a similar message in the SSR from PHP.

It seems for actual rendering on the front it should skip doing anything if an ID is not matched?

DAreRodz · 2021-10-07T14:34:26Z

It seems for actual rendering on the front it should skip doing anything if an ID is not matched?

It's what it does right now, I'll leave as it is then. 👍

DAreRodz · 2021-10-07T19:04:11Z

I've updated the PR description to reflect the changes I've done. Not sure if there's anything I missed, so asking for reviewers. 🙂

DAreRodz · 2021-10-19T09:40:34Z

I think everything is ready now. 😊

gziolo

Code still looks good 👍🏻

DAreRodz · 2021-10-19T09:58:01Z

Did I break something? There's a test failing that hadn't failed before. 👀

gziolo · 2021-10-19T10:05:25Z

Did I break something? There's a test failing that hadn't failed before. 👀

It might be something with the env on CI:

We need to check other PRs to confirm.

Update: It's unrelated to this PR. I see the same issue in other places.

jameskoster · 2021-10-19T10:27:36Z

I see, I'm using TT1 and it works fine with this theme. About CSS and priority, I don't know if there is a better solution than trying to make the CSS rules for type controls more restrictive ― which is far from a perfect solution as themes can use more restrictive rules as well. 🤷 I would love to read other opinions on this.

This is important to get right before merge imo, because in some rare cases none of these settings would work and the block would feel broken.

Ultimately if you're changing a setting on the block, it's because you don't like what is supplied by the theme, so I feel the block should take priority here if possible.

Oh, I guess you tried with an invalid commentId, am I right?

Correct :D It's working now 👍

geriux · 2021-10-19T11:52:19Z

Did I break something? There's a test failing that hadn't failed before. 👀

Hey there 👋 the fix for that issue is now merged into trunk

Settings are: * Typography and color settings (same as the Paragraph block) * Layout settings (same as Post Content block)

I've also removed some cases that I think should be handled in the parent block: - When the comment is not ready yet - When the comment does not exist

Added experimental settings for changing - Font family - Font weight - Font style - Text transform - Letter spacing

gziolo · 2021-10-20T10:30:58Z

I see, I'm using TT1 and it works fine with this theme. About CSS and priority, I don't know if there is a better solution than trying to make the CSS rules for type controls more restrictive ― which is far from a perfect solution as themes can use more restrictive rules as well. 🤷 I would love to read other opinions on this.

This is important to get right before merge imo, because in some rare cases none of these settings would work and the block would feel broken.

Ultimately if you're changing a setting on the block, it's because you don't like what is supplied by the theme, so I feel the block should take priority here if possible.

I’m still unsure if that is a fixable issue with code. Block themes should leave the styling to blocks when it is supported. Maybe it’s only specific to a theme you tested with and we can sort it out by publishing a dev note for theme authors. They might be able to use theme.json to disable the UI controls if they have opinionated rules or remove those conflicting rules from CSS. It looks like the chicken or the egg dilemma 😅

gziolo · 2021-10-20T11:06:27Z

It tests well when using TT1 blocks theme:

Editor:

Frontend:

jameskoster · 2021-10-20T12:39:32Z

Maybe it’s only specific to a theme you tested with and we can sort it out by publishing a dev note for theme authors.

That seems fair.

github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 28, 2021

DAreRodz mentioned this pull request Sep 28, 2021

[Block Library - Post Comment Content]: Fix block's edit function #35190

Merged

ntsekouras reviewed Sep 28, 2021

View reviewed changes

DAreRodz force-pushed the add/comment-content-block branch from b98c625 to 5714fef Compare September 29, 2021 16:39

ntsekouras reviewed Sep 30, 2021

View reviewed changes

packages/icons/src/library/comment-content.js Outdated Show resolved Hide resolved

ntsekouras reviewed Sep 30, 2021

View reviewed changes

DAreRodz force-pushed the add/comment-content-block branch from 5714fef to b5b968a Compare October 1, 2021 10:01

ntsekouras closed this Oct 4, 2021

ntsekouras reopened this Oct 4, 2021

DAreRodz force-pushed the add/comment-content-block branch from b5b968a to bea31f9 Compare October 6, 2021 09:35

DAreRodz commented Oct 6, 2021

View reviewed changes

DAreRodz force-pushed the add/comment-content-block branch from 04abec7 to 7b2eaa3 Compare October 7, 2021 12:12

DAreRodz force-pushed the add/comment-content-block branch from 7b2eaa3 to 6c22993 Compare October 7, 2021 19:00

DAreRodz force-pushed the add/comment-content-block branch from 57f895b to 5404513 Compare October 19, 2021 09:38

gziolo self-requested a review October 19, 2021 09:48

gziolo approved these changes Oct 19, 2021

View reviewed changes

DAreRodz added 14 commits October 19, 2021 18:23

Comment Content Block: add more supports settings

101994e

Settings are: * Typography and color settings (same as the Paragraph block) * Layout settings (same as Post Content block)

Comment Content: Add block icon

4d87f07

Comment Content: Show placeholder when no context

697e778

Comment Content: Add jsdoc to edit component

105e488

Comment Content: Remove the layout setting

202288f

Comment Content: Fix path props in icon

10dfbd8

Comment Content: Check if the comment is ready

813fcc3

Comment Content: Replace loading message with placeholder

a9d6214

Comment Content: Support for textAlign attribute

83fcd22

I've also removed some cases that I think should be handled in the parent block: - When the comment is not ready yet - When the comment does not exist

Comment Content: Add padding support

ec40891

Comment Content: Add more typography settings

a776ca8

Added experimental settings for changing - Font family - Font weight - Font style - Text transform - Letter spacing

Comment Content: Remove unnecessary comments

13c61e6

Icons: Add post prefix to Comment Content icon

15bb5a6

Comment Content: Show block name as placeholder

1fa7d39

DAreRodz force-pushed the add/comment-content-block branch from 5404513 to 1fa7d39 Compare October 19, 2021 16:30

gziolo merged commit c3bf2f5 into WordPress:trunk Oct 20, 2021

github-actions bot added this to the Gutenberg 11.8 milestone Oct 20, 2021

DAreRodz deleted the add/comment-content-block branch October 20, 2021 14:55

michalczaplinski mentioned this pull request Oct 22, 2021

Site Editor: Add new Comments Query Loop block #35231

Merged

7 tasks

	{ __( 'The specified comment does not exist.' ) }
	{ __( 'Comment Content: The specified comment does not exist.' ) }

Comment Content Block: Add typography, color and padding support #35183

Comment Content Block: Add typography, color and padding support #35183

Conversation

DAreRodz commented Sep 28, 2021 • edited by gziolo Loading

Description

How has this been tested?

Types of changes

Checklist:

github-actions bot commented Sep 28, 2021

mtias commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SantosGuillamot Sep 29, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jameskoster commented Sep 28, 2021 • edited Loading

ntsekouras commented Sep 28, 2021

jameskoster commented Sep 28, 2021 • edited Loading

DAreRodz commented Sep 29, 2021

ntsekouras Sep 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ntsekouras commented Oct 4, 2021

DAreRodz commented Oct 5, 2021

DAreRodz commented Oct 6, 2021

Choose a reason for hiding this comment

jameskoster commented Oct 6, 2021 • edited Loading

mtias commented Oct 6, 2021

jameskoster commented Oct 6, 2021

DAreRodz commented Oct 6, 2021

jameskoster commented Oct 6, 2021 • edited Loading

mtias commented Oct 7, 2021

DAreRodz commented Oct 7, 2021

DAreRodz commented Oct 7, 2021

DAreRodz commented Oct 19, 2021

gziolo left a comment

Choose a reason for hiding this comment

DAreRodz commented Oct 19, 2021

gziolo commented Oct 19, 2021 • edited Loading

jameskoster commented Oct 19, 2021

geriux commented Oct 19, 2021

gziolo commented Oct 20, 2021 • edited Loading

gziolo commented Oct 20, 2021 • edited Loading

jameskoster commented Oct 20, 2021

DAreRodz commented Sep 28, 2021 •

edited by gziolo

Loading

SantosGuillamot Sep 29, 2021 •

edited

Loading

jameskoster commented Sep 28, 2021 •

edited

Loading

jameskoster commented Sep 28, 2021 •

edited

Loading

ntsekouras Sep 30, 2021 •

edited

Loading

jameskoster commented Oct 6, 2021 •

edited

Loading

jameskoster commented Oct 6, 2021 •

edited

Loading

gziolo commented Oct 19, 2021 •

edited

Loading

gziolo commented Oct 20, 2021 •

edited

Loading

gziolo commented Oct 20, 2021 •

edited

Loading