-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 Latest Comments block. #7369
Conversation
@melchoyce Here is a video of the Latest Comments block, let me know if you have any concerns about the design: |
core-blocks/latest-comments/edit.js
Outdated
/> | ||
</BlockControls> | ||
<InspectorControls key="inspector"> | ||
<h3>{ __( 'Latest Comments Settings' ) }</h3> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<PanelBody title={ __( 'Latest Comments Settings' ) }>
like in latest-posts looks a little bit nicer.
@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. |
@karmatosed Thanks for checking the video and for the comments.
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: |
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? |
@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: |
This is looking really close to being done @miina. Just one thing I noticed: Should the date clear? |
@karmatosed Made some changes, hopefully it works as expected now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for all the changes and your hard work on this @miina. It was a pleasure working through the process with you.
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
core-blocks/latest-comments/edit.js
Outdated
const MIN_COMMENTS = 1; | ||
const MAX_COMMENTS = 100; | ||
|
||
class latestComments extends Component { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should start with the upper-case. We are adding missing docs for this to make it more formal. See #7670.
core-blocks/latest-comments/edit.js
Outdated
|
||
return ( | ||
<Fragment> | ||
<BlockControls key="controls"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no need to add key
prop when you use Fragment
component wrapper.
core-blocks/latest-comments/edit.js
Outdated
controls={ [ 'left', 'center', 'right', 'wide', 'full' ] } | ||
/> | ||
</BlockControls> | ||
<InspectorControls key="inspector"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key
should be removed. See above.
core-blocks/latest-comments/edit.js
Outdated
/> | ||
</BlockControls> | ||
<InspectorControls key="inspector"> | ||
<PanelBody title={ __( 'Latest Comments Settings' ) } /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess that in this case PanelBody
should wrap all controls like it happens in other blocks.
core-blocks/latest-comments/edit.js
Outdated
/> | ||
|
||
</InspectorControls> | ||
<ServerSideRender key="latest-comments" block="core/latest-comments" attributes={ this.props.attributes } /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
key
should be removed. See above.
|
||
register_block_type( 'core/latest-comments', array( | ||
'attributes' => array( | ||
'className' => array( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.wp-block-latest-comments { | ||
padding-left: 2.5em; | ||
|
||
&__comment-timestamp { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
} | ||
|
||
.wp-block-latest-comments > li { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have styles for the same element in line 10, they should be consolidated there.
padding-top: 8px; | ||
} | ||
|
||
.wp-block-latest-comments__comment-excerpt p { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those styles should be also moved to top level. In general, there is no need to nest classes which use the same pattern.
margin: 5px 0 20px; | ||
} | ||
|
||
.recentcomments { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is recentcomments
something that is named for backward compatibility?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing to latestcomments
, was not intentional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@karmatosed, any recommendations on how to tackle it? |
onChange={ ( nextAlign ) => { | ||
setAttributes( { align: nextAlign } ); | ||
} } | ||
controls={ [ 'left', 'center', 'right', 'wide', 'full' ] } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think those are default values and can be omitted in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, confirmed:
const DEFAULT_CONTROLS = [ 'left', 'center', 'right', 'wide', 'full' ];
} | ||
|
||
toggleHandler( propName ) { | ||
return () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case the whole element re-renders from server-side data though, right? 🤷♂️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
), | ||
'align' => array( | ||
'type' => 'string', | ||
'default' => 'center', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
|
||
getEditWrapperProps( attributes ) { | ||
const { align } = attributes; | ||
if ( 'left' === align || 'right' === align || 'wide' === align || 'full' === align ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be closed in favor of #7941 ? |
Yes. |
Description
Fixes #1792. Note that this PR is based on #1931.
Adds
Latest Comments
block v1 usingServerSideRender
component.Allows configuring the following:
Screenshots
Editor view:
Frontend view:
Checklist: