Comments Query Loop: Update default template #40165
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The fontSize prop default is set to 'small'. The isLink prop is set to true. Blocks with changes are: - Comment Author Name - Comment Date - Comment Edit Link - Comment Reply Link
DAreRodz
added
[Type] Enhancement
DAreRodz
requested review from
michalczaplinski,
ajitbohra,
mkaz,
ryanwelcher and
juanmaguitar
as code owners
|
Size Change: +107 B (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
SantosGuillamot
approved these changes
Apr 7, 2022
michalczaplinski
approved these changes
Apr 7, 2022
| @@ -27,6 +27,9 @@ function render_block_core_comment_author_name( $attributes, $content, $block ) | |||
| if ( isset( $attributes['textAlign'] ) ) { | |||
| $classes .= 'has-text-align-' . esc_attr( $attributes['textAlign'] ); | |||
| } | |||
| if ( isset( $attributes['fontSize'] ) ) { | |||
| $classes .= 'has-' . esc_attr( $attributes['fontSize'] ) . '-font-size'; | |||
There was a problem hiding this comment.
I think there is no need to escape the attributes here, they are already escaped in get_block_wrapper_attributes().
There was a problem hiding this comment.
In this case, we don't use the get_block_wrapper_attributes() function to display that class, I guess that's the reason why we have it there.
| $wrapper_attributes = get_block_wrapper_attributes(); | ||
| $classes = ''; | ||
| if ( isset( $attributes['fontSize'] ) ) { | ||
| $classes .= 'has-' . esc_attr( $attributes['fontSize'] ) . '-font-size'; |
There was a problem hiding this comment.
I think there is no need to escape the attributes here, they are already escaped in get_block_wrapper_attributes().
There was a problem hiding this comment.
Was there an issue created for this? It has not been solved.
There was a problem hiding this comment.
Hey @carolinan, you're right; there's no issue created. 😅 Feel free to open one and address it.
| @@ -31,6 +31,9 @@ function render_block_core_comment_edit_link( $attributes, $content, $block ) { | |||
| if ( isset( $attributes['textAlign'] ) ) { | |||
| $classes .= 'has-text-align-' . esc_attr( $attributes['textAlign'] ); | |||
| } | |||
| if ( isset( $attributes['fontSize'] ) ) { | |||
| $classes .= 'has-' . esc_attr( $attributes['fontSize'] ) . '-font-size'; | |||
There was a problem hiding this comment.
I think there is no need to escape the attributes here, they are already escaped in get_block_wrapper_attributes().
| $classes .= 'has-text-align-' . esc_attr( $attributes['textAlign'] ); | ||
| } | ||
| if ( isset( $attributes['fontSize'] ) ) { | ||
| $classes .= 'has-' . esc_attr( $attributes['fontSize'] ) . '-font-size'; |
There was a problem hiding this comment.
I think there is no need to escape the attributes here, they are already escaped in get_block_wrapper_attributes().
|
Nice, this is a big improvement. Before: After:
A couple of thoughts that can probably all be followed up on: Left column: I wish we could use something other than the Columns block. As created, this becomes a bit rigid; in the GIF above, I try increasing the size of the avatar and it overflows the column. There's a technical question here: should the "Size" option become a "Width" option instead, and have access to unit control? Then, a la how Images work, we could potentially let the resolution of the avatar be available in a dropdown, rather than be inextricably tied to the dimensions of the avatar:
Ultimately this would make it possible for us to set the default width of the avatar block to be Right column: Looks great. The use of the Row block is good. Avatar radius: Right now it's preset to 20px, which of course is half of 40px. That makes it size inevenly. A trick here: if the border-radius size is set to pixels, it will never cause an ovaloid shape. I.e. if you set the radius to |
|
To clarify the columns comment: if the avatar block had a percentage width and scaled to its column, the columns block is arguably the right one to use. It's mainly that there are multiple widths you need to update in order to change the avatar size, that's a bit of a challenge at the moment. |
|
Thanks, @michalczaplinski, and @jasmussen for your useful feedback. I'll work on the suggested changes in a follow-up PR. 😁 |
gziolo
reviewed
Apr 8, 2022
| @@ -160,7 +163,7 @@ function test_rendering_comment_template_nested() { | |||
|
|
|||
| $this->assertEquals( | |||
| gutenberg_render_block_core_comment_template( null, null, $block ), | |||
There was a problem hiding this comment.
The expected value should go first here as well.
This was referenced Apr 20, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Changes the default template to make it more aesthetic. Also, it modifies the default values of some block attributes.
Why?
To let editor users have a first good structure for the Comments Query Loop inner blocks.
How?
innerBlocksfor the Comments Query Loop block.isLinkandfontSizeattributes ofTesting Instructions
Screenshots or screencast