Add Latest Comments block. by miina · Pull Request #7369 · WordPress/gutenberg

miina · 2018-06-19T12:50:55Z

Description

Fixes #1792. Note that this PR is based on #1931.

Adds Latest Comments block v1 using ServerSideRender component.
Allows configuring the following:

Number of comments displayed;
Comment excerpt to be shown;
Permit avatar to be shown;
Permit timestamps to be shown.

Screenshots

Editor view:

Frontend view:

Checklist:

My code is tested functionally.
My code follows the WordPress code style.
My code has proper inline documentation.

…-block

miina · 2018-06-19T12:51:55Z

@melchoyce Here is a video of the Latest Comments block, let me know if you have any concerns about the design:
https://www.dropbox.com/s/dfqt2ixcd8w0oga/latest-comments.mov?dl=0

Soean · 2018-06-19T15:23:49Z

core-blocks/latest-comments/edit.js

+					/>
+				</BlockControls>
+				<InspectorControls key="inspector">
+					<h3>{ __( 'Latest Comments Settings' ) }</h3>


<PanelBody title={ __( 'Latest Comments Settings' ) }> like in latest-posts looks a little bit nicer.

karmatosed · 2018-06-20T11:38:29Z

@miina there seems to be some issue running this. I can't find it in the block library for example. Totally may be something to do with me though. That said, thanks for the video and looking at it there are a few visual issues when the formatting for example right aligns.

Could you please ensure visually it looks a little more like the mocks here? For example spacing and font sizes.

miina · 2018-06-21T12:38:17Z

@karmatosed Thanks for checking the video and for the comments.

I can't find it in the block library for example.

Do you mean that it doesn't show up in the editor blocks? It should be under widgets, let me know if that's not what you meant.

Adjusted the style, here are screenshots with different configuration, let me know if we should make more adjustments:

Comments with just the excerpt:
Comments with date, avatar:
Comments with date, avatar, time:
Right aligned comments:

karmatosed · 2018-06-22T20:05:32Z

Thanks @miina. I have a little design review and once that's done we can look to get this in.

First, when a data isn't show would it be possible to centre the text over have the gap underneath?

Where there is an excerpt there seems too much padding on name but too small line height when the line wraps:

Possibly could we have more space below each comment?

Can the 'number of comments' be a smaller input and on the same line as the label?

miina · 2018-06-23T09:09:48Z

@karmatosed Thanks for the additional feedback. Made some adjustments and also replaced the text control with range control, the same way as Latest Posts block is using the number of items to display:

karmatosed · 2018-07-01T15:16:06Z

This is looking really close to being done @miina. Just one thing I noticed:

Should the date clear?

…-block

…les.

miina · 2018-07-02T10:51:35Z

@karmatosed Made some changes, hopefully it works as expected now.

karmatosed

Thanks for all the changes and your hard work on this @miina. It was a pleasure working through the process with you.

miina · 2018-07-05T08:51:25Z

For information that I'm going to be mostly offline from the 6th until the end of July -- if any changes would be necessary meanwhile to have the PR merged anyone is welcome to pick it up.

gziolo

I left my feedback. I didn't find any bigger issues, mostly regular feedback to ensure code consistency. Let me know if you have any questions.

gziolo · 2018-07-05T08:41:45Z

core-blocks/latest-comments/edit.js

+const MIN_COMMENTS = 1;
+const MAX_COMMENTS = 100;
+
+class latestComments extends Component {


It should start with the upper-case. We are adding missing docs for this to make it more formal. See #7670.

gziolo · 2018-07-05T08:42:19Z

core-blocks/latest-comments/edit.js

+
+		return (
+			<Fragment>
+				<BlockControls key="controls">


There is no need to add key prop when you use Fragment component wrapper.

gziolo · 2018-07-05T08:42:37Z

core-blocks/latest-comments/edit.js

+						controls={ [ 'left', 'center', 'right', 'wide', 'full' ] }
+					/>
+				</BlockControls>
+				<InspectorControls key="inspector">


key should be removed. See above.

gziolo · 2018-07-05T08:43:32Z

core-blocks/latest-comments/edit.js

+					/>
+				</BlockControls>
+				<InspectorControls key="inspector">
+					<PanelBody title={ __( 'Latest Comments Settings' ) } />


I guess that in this case PanelBody should wrap all controls like it happens in other blocks.

gziolo · 2018-07-05T08:43:40Z

core-blocks/latest-comments/edit.js

+					/>
+
+				</InspectorControls>
+				<ServerSideRender key="latest-comments" block="core/latest-comments" attributes={ this.props.attributes } />


key should be removed. See above.

gziolo · 2018-07-05T08:52:09Z

core-blocks/latest-comments/index.php

+
+register_block_type( 'core/latest-comments', array(
+	'attributes'      => array(
+		'className'        => array(


I see some differences between default values here and those specified in defaultAttributes which I commented about before. Those values get propagated from the server to the client, so should be treated as a source of truth.

gziolo · 2018-07-05T08:54:19Z

core-blocks/latest-comments/style.scss

+.wp-block-latest-comments {
+	padding-left: 2.5em;
+
+	&__comment-timestamp {


wp-block-latest-comments __comment-timestamp should be moved to top level as its own definition. It should be also consolidated with rules from lines 23-26.

gziolo · 2018-07-05T08:55:13Z

core-blocks/latest-comments/style.scss

+	}
+}
+
+.wp-block-latest-comments > li {


We have styles for the same element in line 10, they should be consolidated there.

gziolo · 2018-07-05T08:56:07Z

core-blocks/latest-comments/style.scss

+		padding-top: 8px;
+	}
+
+	.wp-block-latest-comments__comment-excerpt p {


Those styles should be also moved to top level. In general, there is no need to nest classes which use the same pattern.

gziolo · 2018-07-05T08:56:59Z

core-blocks/latest-comments/style.scss

+		margin: 5px 0 20px;
+	}
+
+	.recentcomments {


Is recentcomments something that is named for backward compatibility?

Changing to latestcomments, was not intentional.

…-block

tofumatt

I just checked this out and took it for a spin locally: the UX seems good and I think I just have a few tweaks to make before we get this merged.

The only thing to note here is that if a large number of comments are enabled, the block grows extremely large in size, eg:

Even with just six comments, it's a pretty big block:

I think the appropriate behaviour here should be to show maybe 3-5 comments in full, and then a footer on the element in the editor with a "30 more…" or whatever. It could even be clickable to expand all of the comments, but it shouldn't show them by default.

Gutenberg isn't WYSIWYG, and showing a huge list of comments in the editor window isn't useful.

I'm gonna head to bed but I'll push changes in the morning–assuming it's nothing major (eg just style stuff or little tweaks) I'll get this merged and file a follow-up to the issue of a large number of comments in the editor.

gziolo · 2018-07-11T07:07:48Z

The only thing to note here is that if a large number of comments are enabled, the block grows extremely large in size, eg:

I think the appropriate behaviour here should be to show maybe 3-5 comments in full, and then a footer on the element in the editor with a "30 more…" or whatever. It could even be clickable to expand all of the comments, but it shouldn't show them by default.

@karmatosed, any recommendations on how to tackle it?

gziolo · 2018-07-11T07:09:46Z

core-blocks/latest-comments/edit.js

+						onChange={ ( nextAlign ) => {
+							setAttributes( { align: nextAlign } );
+						} }
+						controls={ [ 'left', 'center', 'right', 'wide', 'full' ] }


I think those are default values and can be omitted in here.

yes, confirmed:

const DEFAULT_CONTROLS = [ 'left', 'center', 'right', 'wide', 'full' ];

gziolo · 2018-07-11T07:11:50Z

core-blocks/latest-comments/edit.js

+	}
+
+	toggleHandler( propName ) {
+		return () => {


Ideally, this arrow function should be promoted to a class method and bind to this instead of toggleHandler to avoid unnecessary rerenders.

I'd have to think how to code it differently :)

In this case the whole element re-renders from server-side data though, right? 🤷‍♂️

Wait, scratch that, I see. I think for now it's okay as we use this pattern elsewhere in the codebase, but if it's a performance issue we could see about fixing it everywhere. I'm afraid I've recommended this in the past, not realising the potential for unneeded re-renders...

Yes, I saw it in other places, too. I don't think this is a big issue for leaf components. However, it's something we should try to avoid whenever it doesn't cause too much work.

tofumatt · 2018-07-12T11:13:04Z

core-blocks/latest-comments/index.php

+		),
+		'align'            => array(
+			'type'    => 'string',
+			'default' => 'center',


This was causing blocks with unset alignment to look centre aligned on a new page load. I'll remove it and the default assignment to centre-alignment above.

tofumatt · 2018-07-12T11:28:03Z

core-blocks/latest-comments/index.js

+
+	getEditWrapperProps( attributes ) {
+		const { align } = attributes;
+		if ( 'left' === align || 'right' === align || 'wide' === align || 'full' === align ) {


This doesn't actually include "center", which caused a bug:

I have a fix incoming which actually DRYs this up a bit too:

aduth · 2018-07-18T18:34:02Z

Can this be closed in favor of #7941 ?

tofumatt · 2018-07-18T18:34:41Z

Yes.

Add a block that shows a range of the latest comments. Fixes #1792 See: #7369, #1931

miina added 7 commits June 11, 2018 17:49

Start adding latest comments block.

343068d

Separate edit.js file. Add style

746dafe

Fix style. Fix displaying controls.

3829d17

Add tests. Remove isSelected.

6b2755d

Merge remote-tracking branch 'origin/master' into add/latest-comments…

ac8181d

…-block

Fix style.

ec51b40

Fix alignment bar. Fix style.

8211ff4

miina added 2 commits June 19, 2018 15:56

Fix eslint.

03c5976

Remove redundant attribute definition.

a557c95

miina mentioned this pull request Jun 19, 2018

Add Block: Recent Comments #1792

Closed

Soean reviewed Jun 19, 2018

View reviewed changes

melchoyce requested a review from karmatosed June 19, 2018 15:29

Adjust style. Use PanelBody title.

7c68cbc

miina added 2 commits June 23, 2018 12:01

Adjust style according to feedback.

595370e

Use RangeControl instead of TextControl

1f6b211

miina added 5 commits July 2, 2018 13:01

Adjust style.

b1d5062

Merge remote-tracking branch 'origin/master' into add/latest-comments…

67a312a

…-block

Style fixes.

decc7de

Add a wrapper around comment data to avoid issues in case of long tit…

fcf92d6

…les.

Revert adding clear.

d4a3574

karmatosed approved these changes Jul 3, 2018

View reviewed changes

karmatosed requested a review from gziolo July 3, 2018 21:28

gziolo requested changes Jul 5, 2018

View reviewed changes

gziolo added this to the 3.3 milestone Jul 5, 2018

gziolo added [Feature] Blocks Overall functionality of blocks [Type] Task Issues or PRs that have been broken down into an individual action to take labels Jul 5, 2018

miina added 3 commits July 5, 2018 14:02

Code consistency fixes.

f358447

Merge remote-tracking branch 'origin/master' into add/latest-comments…

3e4626c

…-block

Restore fixture file.

47bd3cb

tofumatt self-assigned this Jul 10, 2018

tofumatt reviewed Jul 11, 2018

View reviewed changes

gziolo reviewed Jul 11, 2018

View reviewed changes

tofumatt reviewed Jul 12, 2018

View reviewed changes

tofumatt mentioned this pull request Jul 12, 2018

Add latest comments block #7941

Merged

3 tasks

tofumatt closed this Jul 18, 2018

tofumatt added a commit that referenced this pull request Jul 19, 2018

Add latest comments block (#7941)

1feed2c

Add a block that shows a range of the latest comments. Fixes #1792 See: #7369, #1931

Conversation

miina commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshots

Checklist:

Uh oh!

miina commented Jun 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karmatosed commented Jun 20, 2018

Uh oh!

miina commented Jun 21, 2018

Uh oh!

karmatosed commented Jun 22, 2018

Uh oh!

miina commented Jun 23, 2018

Uh oh!

karmatosed commented Jul 1, 2018

Uh oh!

miina commented Jul 2, 2018

Uh oh!

karmatosed left a comment

Choose a reason for hiding this comment

Uh oh!

miina commented Jul 5, 2018

Uh oh!

gziolo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tofumatt left a comment

Choose a reason for hiding this comment

Uh oh!

gziolo commented Jul 11, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth commented Jul 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tofumatt commented Jul 18, 2018

miina commented Jun 19, 2018 •

edited

Loading

gziolo Jul 11, 2018 •

edited

Loading

aduth commented Jul 18, 2018 •

edited

Loading