Fix ServerSideRender component bug with Columns block #6571
Conversation
|
|
||
| componentWillMount() { | ||
| this.isStillMounted = true; | ||
| } |
There was a problem hiding this comment.
componentWillMount will be deprecated in a future React release. Could we do this in componentDidMount instead?
https://reactjs.org/docs/react-component.html#unsafe_componentwillmount
|
|
||
| return wp.apiRequest( { path: path } ).then( ( response ) => { | ||
| if ( response && response.rendered ) { | ||
| return apiRequest( { path: path } ).then( ( response ) => { |
There was a problem hiding this comment.
minor: We could write { path } instead of { path: path } here.
| render() { | ||
| const response = this.state.response; | ||
| if ( ! response || ! response.length ) { | ||
| if ( ! response ) { |
There was a problem hiding this comment.
Unrelated to this PR, but should we handle a possible error state here? If the API returns with a 404 or 500 response then we'll be stuck showing a spinner forever.
There was a problem hiding this comment.
All the comments should be addressed within 9df912a, also added a possible handling for error response including displaying the error message from REST API in case of failure.
For example:
Let me know if this approach seems OK.
There was a problem hiding this comment.
Yeah this works for me. It matches the error state that Shared Blocks have which is nice.
Going forward we probably will want to improve the error state for all of our dynamic blocks. For example, we should make sure that they all have actionable error messages and e.g. Try again buttons.
But I don't want that to block this PR so 👍 for now.
| $attributes = array_merge( $block_type->attributes, $layout ); | ||
| } else { | ||
| $attributes = $layout; | ||
| } |
There was a problem hiding this comment.
Hmm, merging layout into the attributes in the controller doesn't feel right to me since it means that the controller is no longer a light layer on top of the WP_Block_Type_Registry API.
Maybe we could do this in a getter on WP_Block_Type? e.g.
class WP_Block_Type {
...
public function get_attributes() {
if ( is_ array( $this->attributes ) ) {
return array_merge( $this->attributes, array(
'layout' => array(
'type' => 'string',
),
) );
}
return $this->attributes;
}
}
|
Thanks @miina—this looks good. Is there a way to test the changes or do we need to wait for the Archives block to be merged? |
|
@noisysocks Tested it also locally with replacing latest posts return logic with |
noisysocks
left a comment
noisysocks
left a comment
There was a problem hiding this comment.
Tested this locally and it fixes the bug. Nice work 👍
Just need to fix up that i18n issue and this is good to go.
| ); | ||
| } else if ( response.error ) { | ||
| return ( | ||
| <Placeholder>{ __( 'Error loading block: ' ) + response.errorMsg }</Placeholder> |
There was a problem hiding this comment.
We'll need to use wp.i18n.sprintf here so that this string is translatable into RTL languages like Hebrew.
<Placeholder>{ sprintf( __( 'Error loading block: %s' ), response.errorMsg ) }</Placeholder>There was a problem hiding this comment.
Thanks for spotting that, added within dcb8048.
| render() { | ||
| const response = this.state.response; | ||
| if ( ! response || ! response.length ) { | ||
| if ( ! response ) { |
There was a problem hiding this comment.
Yeah this works for me. It matches the error state that Shared Blocks have which is nice.
Going forward we probably will want to improve the error state for all of our dynamic blocks. For example, we should make sure that they all have actionable error messages and e.g. Try again buttons.
But I don't want that to block this PR so 👍 for now.
3 participants
Description
<ServerSideRender>component not working withColumnsblock.How has this been tested?
Types of changes
layoutto each block's schema;Placeholdercomponent;