-
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 #7941
Conversation
} | ||
|
||
$list_items_markup .= __( ' on ', 'gutenberg' ); | ||
$list_items_markup .= '<a class="wp-block-latest-comments__comment-link" href="' . esc_url( get_comment_link( $comment ) ) . '">' . get_the_title( $comment->comment_post_ID ) . '</a>'; |
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.
Just checked into this and it looks like get_the_title()
doesn't do any escaping, so this is Dangerous™. I'll have it fixed in a branch and add tests for it.
$attributes['commentsToShow'] = $DEFAULT_COMMENTS_TO_SHOW; | ||
} | ||
|
||
// This filter is documented in wp-includes/widgets/class-wp-widget-recent-comments.php |
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.
Inline comments must end in full-stops, exclamation marks, or question marks
$author_markup .= '<span class="wp-block-latest-comments__comment-author">' . get_comment_author( $comment ) . '</span>'; | ||
} | ||
|
||
$post_title = '<a class="wp-block-latest-comments__comment-link" href="' . esc_url( get_comment_link( $comment ) ) . '">' . esc_html ( _draft_or_post_title( $comment->comment_post_ID ) ) . '</a>'; |
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 a space after esc_html
|
||
$list_items_markup .= sprintf( | ||
/* translators: 1: author name (inside <a> or <span> tag, based on if they have a URL), 2: post title related to this comment */ | ||
__( '%1$s on %2$s' ), |
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.
Text domain ''gutenberg' is missing
core-blocks/latest-comments/edit.js
Outdated
onChange={ this.toggleAttribute( 'displayAvatar' ) } | ||
/> | ||
<ToggleControl | ||
label={ __( 'Display date/time' ) } |
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 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.
Huh, seems right. It used to say "timestamp" which is definitely wrong, but maybe it should be using a better date function. I'll check it out...
// appearing with no title. | ||
require_once( ABSPATH . 'wp-admin/includes/template.php' ); | ||
|
||
$DEFAULT_COMMENTS_TO_SHOW = 5; |
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 is not valid for phpcs, see: https://make.wordpress.org/core/handbook/best-practices/coding-standards/php/#naming-conventions
} | ||
|
||
register_block_type( 'core/latest-comments', array( | ||
'attributes' => 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.
PHPCS warnings, see https://travis-ci.org/WordPress/gutenberg/jobs/403535665
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.
Ah, cheers. They looked weird before with bizarre indentation but I guess that was the right way.
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, it looks a little bit strange.
You can check it local: https://wordpress.org/gutenberg/handbook/reference/coding-guidelines/#php
(I think this is all set for another review.) |
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 taking this on, @tofumatt.
We need to address the case in which there are no comments to show. This is what the user gets now:
core-blocks/latest-comments/edit.js
Outdated
setCommentsToShow( commentsToShow ) { | ||
const { setAttributes } = this.props; | ||
|
||
setAttributes( { commentsToShow: parseInt( commentsToShow, 10 ) || 0 } ); |
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.
Type coercion already happens behind the block API (asType
). We should drop the ad hoc validation here. See Gallery or Columns for examples of core blocks using RangeControl
with no other type validation than the one built in.
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'm not a big fan of setAlignment
and setCommentsToShow
, as they just duplicate setAttributes
.
I also don't love toggleAttribute
, but don't feel strongly enough to oppose.
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.
The Gallery block, for instance, has a lot of specific functions that just call setAttributes
. I think the notion is explicitly setting one prop per control. It's usually the way I'd create functions for a component. 🤷♂️
The alternative is an inline function passed as a prop, right?
<RangeControl
label={ __( 'Number of comments' ) }
value={ commentsToShow }
onChange={ this.setCommentsToShow }
min={ MIN_COMMENTS }
max={ MAX_COMMENTS }
/>
or
<RangeControl
label={ __( 'Number of comments' ) }
value={ commentsToShow }
onChange={ ( value ) => ( this.setAttributes( 'commentsToShow', value ) ) }
min={ MIN_COMMENTS }
max={ MAX_COMMENTS }
/>
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.
Hm, in both I only see drawbacks. 😄 Pick your poison, I say.
a { | ||
cursor: default; | ||
pointer-events: none; | ||
} |
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 poses the same interaction problems as when the Archives block was being developed. We should use the more robust Disabled
element. See 480338c
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.
Nice, I didn't code this bit but will do. 👍
core-blocks/latest-comments/index.js
Outdated
} | ||
}, | ||
|
||
edit: edit, |
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.
(minor): edit,
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 we should have an eslint rule for that if it's important, but I'll change this one. 😄
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.
Nop, totally minor. I prefer the shorthand. ¯_(ツ)_/¯
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 do too… but we should have rules over preferences 😉
I've filed #8009.
core-blocks/latest-comments/index.js
Outdated
getEditWrapperProps( attributes ) { | ||
const { align } = attributes; | ||
|
||
if ( DEFAULT_CONTROLS.includes( 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.
I realize that the way we need to wrangle alignment types and do the getEditWrapperProps
dance is suboptimal, but I'd rather have that addressed separately. Can we keep getEditWrapperProps
here in line with other core blocks and revert exposing the constants?
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.
Related is
#7908 (comment); before this the code copied what many other elements did and ignored centre-alignment, which caused the centre alignment option to do nothing.
I feel like it's better to do this as a starting point here and convert everything else over in #7908. This would be the way we'd want things to go, 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.
This would be the way we'd want things to go, right?
Maybe, maybe not. :) But it deserves its own PR. I'd undo here.
core-blocks/latest-comments/index.js
Outdated
/** | ||
* Internal dependencies. | ||
*/ | ||
import { DEFAULT_CONTROLS } from 'editor/components/block-alignment-toolbar'; |
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.
Per my other comment, we should get rid of this, but worth noting that this would've been a disapproved import (importing private entities across packages).
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 I'm lost–how is it a private entity?
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.
Because it's not a top-level export of editor
, i.e. you wouldn't be able to get it via
import { DEFAULT_CONTROLS } from '@wordpress/editor';
and we avoid importing across blocks in any other fashion (i.e. a module in core-blocks
importing from editor
). There were some exceptions to this due to circular dependencies, but I think they're all but solved.
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.
Aha, gotcha.
Has there been discussion about a custom rule to catch that for us or has it been difficult to implement? Seems quite easy to miss in a review.
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 are rules against import { Bla } from 'editor';
(and any other package), but none for deep access. I'm assuming we don't yet forbit deep access because of those unresolved circular dependencies we had-and-may-still-have. Someone may correct me on this. :)
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.
That won't actually work (eslint throws an error), but I have a PR incoming.
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.
That won't actually work (eslint throws an error), but I have a PR incoming.
Yeah, might be the same thing I've encountered before, where it was solved by including the unicode equivalent character of the slash.
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.
Uncanny! Do you have a matcher for lint?
I actually have no idea how this notification came through to my inbox 😅
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.
Whoa, is that what that is? If it works I'll add a comment for it this time 😅
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.
zomg that worked, thanks. I'll add it and add a comment this time 😉
$author_markup .= '<span class="wp-block-latest-comments__comment-author">' . get_comment_author( $comment ) . '</span>'; | ||
} | ||
|
||
$post_title = '<a class="wp-block-latest-comments__comment-link" href="' . esc_url( get_comment_link( $comment ) ) . '">' . esc_html( _draft_or_post_title( $comment->comment_post_ID ) ) . '</a>'; |
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.
_draft_or_post_title
already calls esc_html
, though I can accept if you'd rather also escape here for reviewers peace of mind.
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 I originally added the esc_html
(or it was here already?) because we weren't using _draft_or_post_title
. I suppose now that it's being used we can omit the extra escape.
In general I like seeing it but in this case I'll leave a comment explaining that _draft_or_post_title
does the escaping. No need to run it twice.
Yeah, I noticed that and I have something in the pipeline, should be pushed soon. |
Thanks for the review, all set for another look. |
aa2bed3
to
6604304
Compare
Wait, scratch that, I seem to be getting a weird CSS error locally. Investigating... |
* chore: Improve eslint checks for deep imports See: #7941 (comment) * chore: Use fetch not request
It was something weird locally. Reset my env and everything is good. Ready for another review! |
Can #7369 be closed in favor of this one? |
Got some test failures, might be a bad rebase with the fixtures. Should be regenerated with |
Indeed, I see those too. Just a moment and I'll push that 👍 |
Well, fixed things manually for now, but I'll need to see why the fixtures aren't regenerating properly later... Should be passing now. |
There are PHPCS (code styling) errors now. Will review what's here currently in the meantime.
|
Shoot, I thought I fixed them, sorry 'bout that. Will push the fix now. |
(Fixed) |
core-blocks/latest-comments/edit.js
Outdated
this.toggleAttribute = this.toggleAttribute.bind( this ); | ||
} | ||
|
||
toggleAttribute( propName ) { |
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 function doesn't toggle anything on its own, it creates a function which performs the toggle. More accurately, it might be named createToggleAttribute
.
core-blocks/latest-comments/edit.js
Outdated
<ToggleControl | ||
label={ __( 'Display avatar' ) } | ||
checked={ displayAvatar } | ||
onChange={ this.toggleAttribute( 'displayAvatar' ) } |
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.
toggleAttribute
returns a new function every invocation, causing React to do render reconciliation more often than we need it to. I wonder if we should pre-bind these functions in the constructor:
constructor() {
// ...
this.toggleDisplayAvatar = this.createToggleAttribute( 'displayAvatar' );
}
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.
Yeah, this has been pointed out elsewhere that this pattern is bad for unneeded re-renders, though obviously the code is nice.
Pre-binding them in the constructor is a good pattern actually 👍
core-blocks/latest-comments/edit.js
Outdated
*/ | ||
import './editor.scss'; | ||
|
||
const MIN_COMMENTS = 1; |
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.
IMO we should JSDoc any constant, even just to establish the convention in encouraging the practice for less-obvious constants.
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'm cool with that 👍
/> | ||
</PanelBody> | ||
</InspectorControls> | ||
<Disabled> |
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.
Makes me wonder: Is there any case where we want ServerSideRender
to not be disabled? Should this be built-in to the render logic of that 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.
I wondered that too, we should probably open an issue for it. I suppose building it in and then having some escape hatch prop that doesn't disable it would be good.
margin-right: 10px; | ||
} | ||
|
||
.edit-post-visual-editor { |
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.
Why do we need this context?
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 bet we don't; I inherited this CSS and I guess I didn't refactor enough. It can go. 👍
// appearing with no title. | ||
require_once( ABSPATH . 'wp-admin/includes/template.php' ); | ||
|
||
$default_comments_to_show = 5; |
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.
As I understand it, scoping in PHP won't work as expected with what we have here. This would need to be accessed by global
, which we're not doing.
Simpler demonstration:
Before:
<?php
$extra = 'bar';
function do_foo() {
echo 'foo' . $extra;
}
do_foo();
⇒ php test.php
foo%
After:
<?php
$extra = 'bar';
function do_foo() {
global $extra;
echo 'foo' . $extra;
}
do_foo();
⇒ php test.php
foobar%
Though, as written, it's more of a constant, which is exemplified in define
.
Though, above all, does this really need to be in the global scope? I think we're over-optimizing at the risk of outside breakage (anything else in the running WordPress page could access this variable, intentionally or not).
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.
Yeah I didn't think using define
was right because I wanted a constant scoped to this file–and the constants created by define
are global... right?
I'll admit that my PHP is wildly rusty and I'm totally unclear what the best practice is for this.
I just want a file-scoped constant. I can remove it if that's a pain to make.
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.
and the constants created by
define
are global... right?
Yes, and so is the previous variable assignment, as can be demonstrated:
diff --git a/core-blocks/latest-comments/index.php b/core-blocks/latest-comments/index.php
index 33c21e41f..092fe08eb 100644
--- a/core-blocks/latest-comments/index.php
+++ b/core-blocks/latest-comments/index.php
@@ -9,6 +9,7 @@
// appearing with no title.
require_once( ABSPATH . 'wp-admin/includes/template.php' );
+$default_comments_to_show = 5;
define( 'GUTENBERG_LATEST_COMMENTS_BLOCK_DEFAULT_TO_SHOW', 5 );
/**
diff --git a/lib/load.php b/lib/load.php
index f1a8b09c6..f8a4b7985 100644
--- a/lib/load.php
+++ b/lib/load.php
@@ -34,3 +34,5 @@ require dirname( __FILE__ ) . '/register.php';
foreach ( glob( dirname( __FILE__ ) . '/../core-blocks/*/index.php' ) as $block_logic ) {
require $block_logic;
}
+
+var_export( $default_comments_to_show ); exit;
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 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.
Yeah, I didn't realise that 😢
If the define
call is too global I'm fine with just hard-coding it in. 🤷♂️
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.
Since it's prefixed enough it shouldn't be a huge concern.
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.
Alternatively, we could consider enhancing our JSON schema support to handle minimum
and maximum
, rely on the validation behavior, and assign the default only once in the block registration.
https://spacetelescope.github.io/understanding-json-schema/reference/numeric.html#range
In fact, since we use REST validation under the hood, I wonder if this is already possible.
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's already supported:
Our usage will trigger the default to take its place when outside bounds:
gutenberg/lib/class-wp-block-type.php
Lines 137 to 146 in 348e463
if ( isset( $attributes[ $attribute_name ] ) ) { | |
$is_valid = rest_validate_value_from_schema( $attributes[ $attribute_name ], $schema ); | |
if ( ! is_wp_error( $is_valid ) ) { | |
$value = rest_sanitize_value_from_schema( $attributes[ $attribute_name ], $schema ); | |
} | |
} | |
if ( is_null( $value ) && isset( $schema['default'] ) ) { | |
$value = $schema['default']; | |
} |
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's also enum
to take care of the align
valid options:
https://spacetelescope.github.io/understanding-json-schema/reference/generic.html#enumerated-values
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.
Huh, nice! Okay, I'll look at that.
Thanks! Ready for another review. |
|
||
// TODO: Use consistent values across the app; | ||
// see: https://github.com/WordPress/gutenberg/issues/7908. | ||
if ( [ 'left', 'center', 'right', 'wide', 'full' ].includes( 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.
We should double check that our new ES2015 prototype member polyfills with Babel 7 useBuiltIns: 'usage'
allows this to work as expected in IE11.
This is an ES2015+ method which until recently was not expected to be polyfilled and would therefore error in IE11.
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 tested it in IE11 and it worked; I could change the align values and nothing errored on me (it touches this code path).
// appearing with no title. | ||
require_once( ABSPATH . 'wp-admin/includes/template.php' ); | ||
|
||
$default_comments_to_show = 5; |
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 $default_comments_to_show
variable is unused. I suspect it wasn't intentional to keep.
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.
Entirely not, sorry. Fixed.
Side note: I have e2e tests written and ready for this, but they depend on #8041 because I need to ensure there are no existing posts/comments in the test database. |
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.
Looks good 👍
One question. Who will use it ? I am bit confuse with your plans to make blocks for all old WP native widgets. |
The future is blocks :-)
The long-term plan is to have all pieces of a site be built using blocks, so a block like this (or latest posts) can be using where a widget used to go.
- Matt (Sent from mobile)
… On 20 Jul 2018, at 20:31, StaggerLeee ***@***.***> wrote:
One question. Who will use it ?
It is not anything for the Post. Latest comments is thing for Widget.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub, or mute the thread.
|
Description
Fixes #1792. Note that this PR is based on #7369, which is based on #1931. I've made a few tweaks (mentioned in #7369).
Closes #7369.
@tofumatt here:
I've made some changes here from the original PR, mainly around:
get_the_title
to_draft_or_post_title
as it's the only way to display posts with no titleI was going to add tests but these tests require post comments, and making them means I need a mock/test database, as the current e2e tests run against the local development URL. I'll do those in a follow-up branch as I've tested this locally a fair bit.
Adds
Latest Comments
block v1 usingServerSideRender
component.Allows configuring the following:
Screenshots
Editor:
Frontend:
Checklist: