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 a Latest Comments block #1931

Closed
wants to merge 15 commits into from
Closed

Add a Latest Comments block #1931

wants to merge 15 commits into from

Conversation

danielbachhuber
Copy link
Member

@danielbachhuber danielbachhuber commented Jul 17, 2017

  • Permit number of comments displayed to be configurable.
  • Permit comment excerpt to be shown.
  • Permit avatar to be shown.
  • Permit timestamps to be shown.
  • Register server-side block for rendering comments on frontend.
  • Handle truncated comments as excerpts in editor.

See #1792

@danielbachhuber danielbachhuber changed the title Add a Latest Comments block [WIP] Add a Latest Comments block Jul 17, 2017
@mtias mtias added [Feature] Blocks Overall functionality of blocks New Block Suggestion for a new block [Status] In Progress Tracking issues with work in progress labels Jul 25, 2017
Copy link
Member Author

@danielbachhuber danielbachhuber left a comment

Choose a reason for hiding this comment

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

@westonruter @melchoyce This is ready for an initial review. Here's how it looks:

image

super( ...arguments );
this.toggleDisplayAvatar = this.toggleDisplayAvatar.bind( this );
this.toggleDisplayExcerpt = this.toggleDisplayExcerpt.bind( this );
this.toggleDisplayTimestamp = this.toggleDisplayTimestamp.bind( this );
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice if there was some common abstraction for toggling boolean attributes.

Copy link
Member

Choose a reason for hiding this comment

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

How about this? 275b7d9

Copy link
Member Author

Choose a reason for hiding this comment

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

👌

/>

<ToggleControl
label={ __( 'Display timestamp' ) }
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be "Display timestamp" or "Display date"? It'd be nice to have language consistent with posts.

Copy link
Member

Choose a reason for hiding this comment

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

I think it should be “Display publish date” both here and on the Latest Posts block.

Copy link
Member

Choose a reason for hiding this comment

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

Or yeah, just “Display date” would be fine I think.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it should be “Display publish date” both here and on the Latest Posts block

Should I change this in a follow-up PR?

<ul className={ this.props.className } key="latest-comments">
{ latestComments.map( ( comment, i ) =>
<li key={ i }>
{ displayAvatar && comment.author_avatar_urls[ 96 ] &&
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this 96 a safe hard-coded assumption? Or, should we use the largest avatar size on the object?

Copy link
Member

Choose a reason for hiding this comment

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

No, it's not safe because rest_avatar_sizes can be filtered to cause different sizes to be returned in the response. So yeah, using the largest size I think is the right move.

{ displayAvatar && comment.author_avatar_urls[ 96 ] &&
<img className={ `${ this.props.className }__comment-avatar` } alt={ comment.author_name } src={ comment.author_avatar_urls[ 96 ] } />
}
<a href={ comment.link } target="_blank">{ comment._embedded.up[ 0 ].title.rendered }</a>
Copy link
Member Author

Choose a reason for hiding this comment

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

What should we display if there's no linked post, or the linked post doesn't have a title?

Copy link
Member

Choose a reason for hiding this comment

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

When there is no title, see 813af21.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated in f3aa112f

<a href={ comment.link } target="_blank">{ comment._embedded.up[ 0 ].title.rendered }</a>
{ displayTimestamp && comment.date_gmt &&
<span className={ `${ this.props.className }__comment-timestamp` }>
{ moment( comment.date_gmt ).local().format( 'MMM DD h:mm A' ) }
Copy link
Member Author

Choose a reason for hiding this comment

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

This timestamp format seems like it should be user-configurable. Maybe we need a JS implementation of human_time_diff() with a filterable format.

Copy link
Member

Choose a reason for hiding this comment

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

Moment.js does support relative time: https://momentjs.com/docs/#/displaying/fromnow/

It could be a nice block attribute to decide whether the dates are shown in relative time or absolute time, according to the site's datetime format.

See also #1992 (comment) and #1992 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

according to the site's datetime format.

Should we get a new issue going about respecting the site's datetime format?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it affects latest posts as well.

</span>
}
{ displayExcerpt && comment.content &&
<div className={ `${ this.props.className }__comment-excerpt` } dangerouslySetInnerHTML={ { __html: comment.content.rendered } } />
Copy link
Member Author

Choose a reason for hiding this comment

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

What algorithm would you like to use for truncating comments? WordPress doesn't currently implement a comment excerpt.

Copy link
Member

@westonruter westonruter Jul 25, 2017

Choose a reason for hiding this comment

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

Why the dangerouslySetInnerHTML here? Because a user may have markup in their comment?

I am not aware of an existing truncation solution, other than to use CSS.

Copy link
Member Author

Choose a reason for hiding this comment

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

Why the dangerouslySetInnerHTML here? Because a user may have markup in their excerpt?

Yes, HTML is permitted in comments (and WordPress includes some by default anyway)

Copy link
Member

Choose a reason for hiding this comment

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

@mtias thoughts on whether raw HTML from comments should be embedded?

height: 48px;
width: 48px;
border-radius: 24px;
margin-right: 5px;
Copy link
Member Author

Choose a reason for hiding this comment

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

Is it fine that these sizes are in px? If not, can you give me the requested values as em?

Copy link
Member

Choose a reason for hiding this comment

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

There are 30 instances of em units being used in block CSS, whereas there are 37 instances of px units being used. So I'd say px is safe. 👍

"displayTimestamp": true
}
}
]
Copy link
Member Author

Choose a reason for hiding this comment

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

It'd be nice to set up mock data so we can actually test component rendering.

@melchoyce
Copy link
Contributor

Very cool — works as expected to me. Once the your code points are reviewed, let's commit and I'll see if I can figure out a follow-up PR to tweak the design/CSS.

@danielbachhuber danielbachhuber changed the title [WIP] Add a Latest Comments block Add a Latest Comments block Jul 25, 2017
@westonruter
Copy link
Member

Found a styling issue with floats when the avatar alone is displayed:

image

@danielbachhuber
Copy link
Member Author

Found a styling issue with floats when the avatar alone is displayed:

Does this need to be handled in this PR?

@westonruter
Copy link
Member

Does this need to be handled in this PR?

Sure. Why not?

Also, one other big piece needed in this PR is the server-side component for rendering the comments when the block is displayed on the frontend. See #2014 which just refactored where the PHP logic is located.

@westonruter
Copy link
Member

Rebased with master. Rewrote 275b7d93 as aab2f81b.

@westonruter
Copy link
Member

westonruter commented Aug 1, 2017

In the server-side block rendering, I'm confused as to why the $attributes passed to gutenberg_render_block_core_latest_comments() seem to ignore the attributes as provided in the content.

@mtias mtias removed the [Status] In Progress Tracking issues with work in progress label Aug 1, 2017
@westonruter
Copy link
Member

@melchoyce here's how the block is now rendering in the editor:

image

And how it renders on the frontend:

image

Last major piece remaining is how to handle comment excerpts in the editor.

@melchoyce
Copy link
Contributor

@westonruter Cool 👍 I have some visual tweaks I still want to make, but those can happen in a followup PR.

Do you know if there's anything we can do to have the front-end view of the comments block inherit the theme's comment styles, or are we out of luck there?

@westonruter
Copy link
Member

@melchoyce:

Do you know if there's anything we can do to have the front-end view of the comments block inherit the theme's comment styles, or are we out of luck there?

Yeah, I think it is possible. We'd essentially need to use the wp_list_comments() function (template tag) instead of manually crafting our own list. Or at least, we'd need to craft the list to use the same markup and class names (e.g. .comment-body and .comment-author) that the comment walker outputs, since actually wp_list_comments() only lists comments for the current queried post and not all posts across the site.

So that brings up another consideration. There's really two separate comment blocks. One block for the latest comments across the site (as built in this PR) and another block for the latest comments for this current post. This latter one I could see useful to show at the top of a longform post in a call-to-action to comment, or even in template building such a block could be used instead of comments.php entirely. This latter one would then also need to take into account threading and the integration of the comment form.

/**
* WordPress dependencies
*/
import { Component } from 'element';
Copy link
Member

Choose a reason for hiding this comment

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

Note with #2172, these need to be updated to prefix the dependencies with @wordpress/. You will need to perform a rebase against the latest version of master and apply your changes:

git fetch origin
git rebase origin/master

@westonruter
Copy link
Member

Rebased and added dependency prefixes. Rewrote e34aeeb as e0e9088.

@codecov
Copy link

codecov bot commented Aug 3, 2017

Codecov Report

Merging #1931 into master will decrease coverage by 0.2%.
The diff coverage is 7.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1931      +/-   ##
==========================================
- Coverage   22.99%   22.79%   -0.21%     
==========================================
  Files         141      143       +2     
  Lines        4370     4426      +56     
  Branches      738      748      +10     
==========================================
+ Hits         1005     1009       +4     
- Misses       2841     2883      +42     
- Partials      524      534      +10
Impacted Files Coverage Δ
blocks/library/latest-comments/data.js 0% <0%> (ø)
blocks/library/latest-comments/index.js 7.54% <7.54%> (ø)
editor/enable-tracking-prompt/index.js 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51466b2...e0e9088. Read the comment docs.

@westonruter
Copy link
Member

Working on the PHP rendering of this block has confirmed for me the painfulness of having to implement two separate rendering systems, in PHP and JS: the former using normal/ubiquitous WordPress template tags and PHP API function calls, and the latter in React with calls to the REST API. I would not be able to suggest developers look at this block for a recommended streamlined way to write a dynamic block, given the violation of DRY. Maintaining two separate rendering systems with an aim to have the editor be a faithful preview of the frontend will be tedious.

@mtias
Copy link
Member

mtias commented Aug 4, 2017

Are we good with merging this as a first pass?

I would not be able to suggest developers look at this block for a recommended streamlined way to write a dynamic block, given the violation of DRY.

This is only repeating yourself if the editor is not offering editing capabilities for the block, in which case, by all means, people are free to server render it. If you have to write a native mobile block you will need to recreate the rendering as well, that's just the nature of optimizing for each experience.

In the case of widgets, is there a reason why the already built rendering can't be used on the back-end? That means only writing the editor implementation.

@westonruter
Copy link
Member

@mtias:

Are we good with merging this as a first pass?

The only thing not implemented is truncating comment excerpts in the editor, and outputting the comment HTML in a safe way.

This is only repeating yourself if the editor is not offering editing capabilities for the block, in which case, by all means, people are free to server render it. If you have to write a native mobile block you will need to recreate the rendering as well, that's just the nature of optimizing for each experience.

I guess it depends on what you mean by “editing capabilities”. Like is toggling whether or not the date is rendered such an edit?

In the case of widgets, is there a reason why the already built rendering can't be used on the back-end? That means only writing the editor implementation.

This is exactly what I've been suggesting, I think. Using the built-in server-side PHP templating inside the editor block—the same logic that is used to render the dynamic block on the frontend—is what I think we should do in order to eliminate writing two separate renderers. In this way, the a dynamic widget block in the editor would be very similar to what was implemented for shortcode previews in the Shortcake plugin. It would also be very similar to how changes to server-rendered partials are previewed in the Customizer via selective refresh.

@mtias
Copy link
Member

mtias commented Aug 7, 2017

Using the built-in server-side PHP templating inside the editor block—the same logic that is used to render the dynamic block on the frontend—is what I think we should do in order to eliminate writing two separate renderers.

This is not what I mean. I mean using the already built server-side implementation as the front-end rendering instead of redoing all that again, and only creating the editor one from scratch. Server rendering blocks in the editor context doesn't seem a good direction, only a shortcut that would worsen the editing experience in favor of the development experience of only writing once.

@westonruter
Copy link
Member

@mtias: For the Recent Comments widget in core, its server-side rendering logic doesn't itself specifically have filters to modify its markup. But another “Comments for this Post” block (as described above) would indeed have filters since it would use wp_list_comments() which themes and plugins can modify the output of via the wp_list_comments_args filter (even supplying a completely custom Walker_Comment), besides all of the filters that the comment form itself supports in comment_form().

So for the Latest Comments widget block, a client-side React implementation of the UI can indeed be implemented to mirror whatever rendering logic is developed on the server, but I fear this is not going to be scalable or possible in many cases. Mirroring implementations is re-work (not DRY). But if a theme could override a server-side renderer of a Latest Comments block to do something different, and in that case would it be expected that a theme must also create a custom block with its own client-side renderer as well?

If not, and in the case of a “Comments for this Post” block, the source of truth for the block's rendering is the server-side logic. There would not be a way to get a true preview of what the block will render without asking the server to render it. For static blocks, the source of truth is the client-side renderer, so they are kept DRY. For dynamic blocks, this isn't the case. So I don't see server-side rendering of blocks as being a shortcut. It's about not creating more work than is required. It's about honoring the source of truth for what a dynamic block renders, and it's about giving a faithful preview of what the block actually will render.

If many of a dynamic block's settings are purely stylistic, there needn't be a worsening of the editing experience as the client and server in that case could just toggle classes on the wrapper. Otherwise, if an attribute change does require a re-rendering then this is not really a fundamental change from what we have with a client-side rendering because in the case of client-side rendering there is still often a need to refetch data from the API, and so there will often be latency in that case as well.

I think using the server-side rendering of blocks in the editor will be key for developer adoption. Nobody wants to do work twice. It's also important for user adoption, because users want to preview what they're actually going to see rendered on the frontend.

@danielbachhuber
Copy link
Member Author

I think using the server-side rendering of blocks in the editor will be key for developer adoption. Nobody wants to do work twice.

Just to throw an idea in because I haven't seen it discussed: what about a common templating implementation (e.g. Mustache) that can be used both server-side and client-side?

@aduth
Copy link
Member

aduth commented Aug 30, 2017

Maybe of interest for consolidating/automating server-registered block attributes: #2529

@westonruter
Copy link
Member

@danielbachhuber The adoption of templating languages in Core has historically always been shied away from (e.g. Smarty, Twig, etc), the exception being Underscore templates in JS. WordPress uses “pure” PHP templates, for better or worse, and this is why JSX has been viewed so favorably so as to not be using any templating language. Adding a language-agnostic templating language would make the development experience worse on both the client and the server (lack of logic, lack of virtual DOM). I want to open a try PR fork of this branch that uses the Shortcake approach with DRY server-side rendering of the the dynamic block.

@danielbachhuber
Copy link
Member Author

Closing this PR for now. Feel free to re-open when you're ready to proceed on it.

@miina miina mentioned this pull request May 3, 2018
@miina miina mentioned this pull request Jun 19, 2018
3 tasks
@tofumatt tofumatt mentioned this pull request Jul 12, 2018
3 tasks
tofumatt added a commit that referenced this pull request Jul 19, 2018
Add a block that shows a range of the latest comments.

Fixes #1792
See: #7369, #1931
Tug pushed a commit that referenced this pull request Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks New Block Suggestion for a new block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants