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 Server Side Render component. #5602

Merged
merged 32 commits into from
Apr 26, 2018

Conversation

miina
Copy link
Contributor

@miina miina commented Mar 14, 2018

Description

Fixes #780: Adds ServerSideRender component and /gutenberg/v1/blocks-renderer/:block API endpoints for dynamic blocks which together enable server side rendering for dynamic blocks in Gutenberg editor. API endpoint is leveraging the already existing render_callback of the block and returns identical HTML output.

Types of changes

  • Adds ServerSideRender component which can be used for dynamic blocks for server side render. Usage example: <ServerSideRender block="core/latest-posts" attributes={ attributes } />.
  • Adds /gutenberg/v1/blocks-renderer/:block API endpoints, expects the block to be in the format of namespace/block-name.

How Has This Been Tested?

Has been tested functionally with core/archives and also phpunit.

Screenshots:

Archives block with ServerSideRender component:
screen shot 2018-04-06 at 5 22 06 pm
screen shot 2018-04-06 at 5 22 22 pm
screen shot 2018-04-06 at 5 23 02 pm
screen shot 2018-04-06 at 5 23 29 pm

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

Thoughts are welcome!

@miina
Copy link
Contributor Author

miina commented Mar 14, 2018

Question: at this moment the change in attributes is detected by using componentWillReceiveProps -- is there a better way to bind the change of props to updating the preview?

...attributes,
_wpnonce: wpApiSettings.nonce,
} );
return window.fetch( apiURL, {
Copy link
Contributor

Choose a reason for hiding this comment

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

May want to use wp.apiRequest for consistency. See #5253.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think more of a guide on using would help with that. I was working on another PR that used wp.apiRequest and it was leading to test failures (maybe it was how #4710 used in redux connect) so I removed, tests started passing and I could move forward. The only reason my modifications caused issues using wp.apiRequest as far as I could tell was that the tests themselves were not being passed enough context.

@miina miina changed the title Add Server Side Render component [WIP] Add Server Side Render component. Mar 14, 2018
@mtias mtias requested a review from a team March 14, 2018 14:59
@mtias mtias added [Feature] Block API API that allows to express the block paradigm. Framework Issues related to broader framework topics, especially as it relates to javascript Needs Testing Needs further testing to be confirmed. labels Mar 14, 2018
Render core/latest-posts preview.
```jsx
<ServerSideRender
block="core/latest-posts"
Copy link
Member

Choose a reason for hiding this comment

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

We should probably register another block for something that makes more sense server-rendered (maybe tag cloud widget?) to use as the example.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add Archives block from #5495. It's otherwise ready except for the preview which was waiting for SSR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtias For usage example I merged the code from this PR to #5495 which adds Archives block -- thought that this way this PR wouldn't have to wait for potential fixes for the Archives block.

So basically #5495 now includes adding Archives block + SSR component with the idea that SSR component could be merged separately first.

Let me know if that's OK or if you'd prefer having the Archives block added here under this PR.

cc @westonruter

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mtias Updated the description with added screenshots of Archives block based on SSR. Let me know if there's anything else you see in the PR that requires addressing.

*
* @see WP_REST_Controller
*/
class WP_REST_Blocks_Renderer_Controller extends WP_REST_Controller {
Copy link
Member

Choose a reason for hiding this comment

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

Should this be singular? Block_Renderer...

Copy link
Member

Choose a reason for hiding this comment

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

Yah, same with the endpoint route.

Copy link
Member

Choose a reason for hiding this comment

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

Done in c4abc69

return new WP_Error( 'rest_block_invalid_name', __( 'Invalid block name.', 'gutenberg' ), array( 'status' => 404 ) );
}

$atts = $this->prepare_attributes( $request->get_params() );
Copy link
Member

Choose a reason for hiding this comment

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

Since we have $block here, this means that we have $block->attributes which can be used with rest_validate_value_from_schema() and rest_sanitize_value_from_schema(). For example:

$atts = array();
$schema = $block->attributes;
foreach ( $request->get_params() as $key => $value ) {
	if ( ! isset( $schema[ $key ] ) ) {
		return new WP_Error( 'rest_unrecognized_block_attribute' /* ... */ );
	}
	$validity = rest_validate_value_from_schema( $value, $schema[ $key ], $key );
	if ( is_wp_error( $validity ) ) {
		return $validity;
	}
	$atts[ $key ] = rest_sanitize_value_from_schema( $value, $schema[ $key ] );
}

This should eliminate the need for prepare_attributes entirely.

public function register_routes() {

// @codingStandardsIgnoreLine - PHPCS mistakes $this->namespace for the namespace keyword.
register_rest_route( $this->namespace, '/' . $this->rest_base . '/(?P<name>[\w-]+\/[\w-]+)', array(
Copy link
Member

Choose a reason for hiding this comment

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

One idea: instead of letting name be a parameter, we could register over all of the registered blocks and create an endpoint for each one. This could then allow for the block's attributes schema to be used as an attributes object arg in the args here, and it would eliminate the need for prepare_attributes logic in get_item_output. (See https://core.trac.wordpress.org/ticket/38583)

This would allow for a list of all the server-side renderable blocks to be discovered by looking at the REST API schema. Only the blocks returned by get_dynamic_block_names() would be included.

'title' => 'blocks-renderer',
'type' => 'object',
'properties' => array(
'output' => array(
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 the rendered name would be more in-line with the rest of the REST API.

Copy link
Member

Choose a reason for hiding this comment

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

Done in c4abc69

}

return (
<RawHTML key="html">{ response }</RawHTML>
Copy link
Member

Choose a reason for hiding this comment

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

Should this use SandBox instead?

Copy link
Member

Choose a reason for hiding this comment

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

I guess not because then styles won't be applied.

Copy link
Contributor

@Lewiscowles1986 Lewiscowles1986 Mar 15, 2018

Choose a reason for hiding this comment

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

@westonruter Sandbox doesn't stop styles being applied (If I understand correctly it ensures no css / js bleeds into admin experience). It's used in #4710. Should work on such block-specifics defer to this once it's released?

Copy link
Member

Choose a reason for hiding this comment

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

I mean the CSS from the editor. Otherwise, the CSS has to be re-printed in the iframe.

}

componentWillReceiveProps( nextProps ) {
if ( JSON.stringify( nextProps ) !== JSON.stringify( this.props ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

What about using isEqual from lodash?

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I was going to comment that there was room to figure out how aggressive this component should be with refetches and ask if a shallow equality check could make any sense. However, by design, block attributes should (until abused) retain a very simple shape. isEqual should do it.

  2. More importantly, it's not clear to me why there is state synchronization at all. Perhaps this is a relic of a previous solution? response appears to be the only datum requiring local state. Component props should be fed directly into getOutput. If the goal is to trigger a refetch and a rerender, the following should be enough:

componentWillReceiveProps( nextProps ) {
  if ( ! isEqual( nextProps, this.props ) ) {
    this.getOutput();
  }
}

…t of the REST API

* Singularize "blocks renderer" to "block renderer"
* Rename 'output' property to 'rendered' property.
* Re-use get_item_permissions_check and get_item method names.
* @return WP_REST_Response|WP_Error Response object on success, or WP_Error object on failure.
*/
public function get_item( $request ) {
if ( ! isset( $request['name'] ) ) {
Copy link
Member

@pento pento Mar 14, 2018

Choose a reason for hiding this comment

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

The regex for name in the register_rest_route() call means $request['name'] should never be not set.

@westonruter
Copy link
Member

I'm working on refactoring how the routes are registered. Will be pushing up within the hour.

* Supply block attributes schema as endpoint schema.
* Introduce attributes endpoint property and let REST API schema validate and sanitize them.
* Ensure that attribute values are sanitized in addition to validated.
$registry = WP_Block_Type_Registry::get_instance();
$block = $registry->get_registered( $request['name'] );
$data = array(
'rendered' => $block->render( $request->get_param( 'attributes' ) ),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@westonruter This isn't working with the current SSR component code since the attributes are all sent as separate params and not as one attributes array:

                const apiURL = addQueryArgs( '/gutenberg/v1/block-renderer/' + block, {
			...attributes,
			_wpnonce: wpApiSettings.nonce,
		} );

Just sending attributes: attributes as param would fail the request, is there an existing method that creates an acceptable query string for the API (e.g. ?attributes[key1]=value1&attributes[key2]=value2... etc? If not then perhaps we could still pass all the attributes as separate params and modify the endpoint accordingly. Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

That's a good question. I think it's important for attributes to be under an attributes query parameter because that will prevent conflicts with block attributes. Like if a block has a name or a context these will conflict with the REST API args. It seems like this should be allowed:

const apiURL = addQueryArgs( '/gutenberg/v1/block-renderer/' + block, {
	attributes,
	_wpnonce: wpApiSettings.nonce,
} );

In other words, I think addQueryArgs needs to be able to properly convert arg values that are objects.

Copy link
Contributor Author

@miina miina Mar 16, 2018

Choose a reason for hiding this comment

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

Couldn't find an existing method at this moment, apparently query-string is intentionally not supporting nested attributes and suggests sending the object as a JSON string.

Added a custom method for now to the class to put together the query string supporting objects (ServerSideRender.getQueryUrlFromObject).


ServerSideRender component is used for server-side rendering preview in Gutenberg editor, specifically for dynamic blocks. Server-side rendering in a block's `edit` function should be limited for blocks which are heavily dependent on (existing) PHP rendering logic that is heavily intertwined with data, such as when there are no endpoints available.

New blocks should be built in conjunction with any necessary REST API endpoints so that JavaScript can be used for rendering client-side in the `edit` function for the best user experience, instead of relying using the PHP `render_callback`. The logic necessary for rendering should be included in the endpoint so that both the client-side JS and server-side PHP logic should require a mininal amount of differences.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/relying using/relying on/

ServerSideRender
=======

ServerSideRender component is used for server-side rendering preview in Gutenberg editor, specifically for dynamic blocks. Server-side rendering in a block's `edit` function should be limited for blocks which are heavily dependent on (existing) PHP rendering logic that is heavily intertwined with data, such as when there are no endpoints available.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it's important to stress two other conditions to prefer SSR, more significant, I dare say, than the logic/data entanglement, since that isn't of itself insurmountable:

  • A lack of potential to take some existing widget-like functionality and improve its user-facing editing experience. For instance, even Archive could benefit from a full JS-driven editing mode: similarly to how Latest Posts offers presentation tweaks (e.g., whether to display as a grid, to show publishing date) or beyond (e.g., allowing tweaking an entry's title, changing its date or otherwise moving it out of that particular archive view).
  • An unwillingness to create a full JS experience for a block considered legacy.

The ultimate point, perhaps, is that—however useful—SSR should be regarded as a fallback.


## Usage

Render core/latest-posts preview.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/latest-posts/archives/


## Output

Output is using the `render_callback` set when defining the block. For example if `block="core/latest-posts"` as in the example then the output will match `render_callback` output of that block.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/latest-posts/archives/

}

componentWillReceiveProps( nextProps ) {
if ( JSON.stringify( nextProps ) !== JSON.stringify( this.props ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I was going to comment that there was room to figure out how aggressive this component should be with refetches and ask if a shallow equality check could make any sense. However, by design, block attributes should (until abused) retain a very simple shape. isEqual should do it.

  2. More importantly, it's not clear to me why there is state synchronization at all. Perhaps this is a relic of a previous solution? response appears to be the only datum requiring local state. Component props should be fed directly into getOutput. If the goal is to trigger a refetch and a rerender, the following should be enough:

componentWillReceiveProps( nextProps ) {
  if ( ! isEqual( nextProps, this.props ) ) {
    this.getOutput();
  }
}

}
}

getOutput() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a name like fetch more apt?

constructor( props ) {
super( props );
this.state = {
response: {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the empty object as a representation of missing state a pattern from somewhere else? I'd expect null or undefined to be a better fit. This works well with the fact that the request resolution already checks for response truthiness:

if ( response && response.output ) {

Meaning that we can easily distinguish the three possible states, e.g.:

  • no content: response === null
  • content missing: response !== null && isEmpty( response )
  • count found: ! isEmpty( response )


/**
* NA.
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd drop the placeholder comments, since they can throw our linter off and later slip through review.

* @access public
*/
public function __construct() {

Copy link
Contributor

Choose a reason for hiding this comment

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

Per the project's style guide, function bodies shouldn't start with an empty line. The same applies to other occurrences in the diff.

Copy link
Member

@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.

👍 Looking pretty good on my end. Left some minor nit picks and then I think we're good for merge.

* @access public
*/
public function __construct() {
// @codingStandardsIgnoreLine - PHPCS mistakes $this->namespace for the namespace keyword.
Copy link
Member

Choose a reason for hiding this comment

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

Can we ignore this in the ruleset?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an exception for this file within 6d1ee65.

continue;
}

// @codingStandardsIgnoreLine - PHPCS mistakes $this->namespace for the namespace keyword.
Copy link
Member

Choose a reason for hiding this comment

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

Ditto above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added an exception for this file within 6d1ee65.

public function get_item( $request ) {
global $post;

$post_ID = isset( $request['post_id'] ) ? intval( $request['post_id'] ) : 0;
Copy link
Member

Choose a reason for hiding this comment

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

Can we change the casing of $post_ID to $post_id ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within 6d1ee65.

}

$data = array(
'rendered' => $block->render( $request->get_param( 'attributes' ) ),
Copy link
Member

Choose a reason for hiding this comment

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

We may also be passing the full block content to the render callback in #6239

Rather than needing to remember passing all supported arguments to the render() callback, it would be useful to have a helper function for this.

$response = $this->server->dispatch( $request );

$this->assertErrorResponse( 'gutenberg_block_cannot_read', $response, rest_authorization_required_code() );
}
Copy link
Member

Choose a reason for hiding this comment

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

Can we include additional tests for test_get_item_without_permissions_invalid_post() and test_get_item_without_permissions_cannot_edit_post(), just to make sure we have those code paths covered?

@danielbachhuber
Copy link
Member

Re-requesting a review from @mcsf, because I'd like to make sure his prior comments were addressed to his satisfaction.

I've created a new issue for batch previews in #6381

@westonruter Can you speak briefly to how server block validation (#4063) relates to this endpoint?

@danielbachhuber danielbachhuber requested a review from mcsf April 24, 2018 13:04
@danielbachhuber danielbachhuber added this to the 2.8 milestone Apr 26, 2018
Copy link
Member

@pento pento left a comment

Choose a reason for hiding this comment

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

This is good to go. I'll merge it shortly.

@pento pento merged commit d7ddb69 into WordPress:master Apr 26, 2018
const path = '/gutenberg/v1/block-renderer/' + block + '?context=edit&' + this.getQueryUrlFromObject( { attributes } );

return wp.apiRequest( { path: path } ).then( ( response ) => {
if ( response && response.rendered ) {
Copy link
Member

Choose a reason for hiding this comment

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

There's nothing to say that the component is still mounted by the time this response is received, so attempting to setState will result in errors. If anything asynchronous is to occur in a component, it must be complemented by a cancellation in componentWillUnmount. It's a bit burdensome, but also speaks to the bigger concern that handling asynchronous behaviors in components is knowingly hard to maintain (and why extracting these behaviors out into e.g. data/core-data avoids this burden being placed on the component author and enables reuse)..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571.

const response = this.state.response;
if ( ! response || ! response.length ) {
return (
<div key="loading" className="wp-block-embed is-loading">
Copy link
Member

Choose a reason for hiding this comment

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

For what reason do we need a key?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with Placeholder within #6571.

return (
<div key="loading" className="wp-block-embed is-loading">

<p>{ __( 'Loading...' ) }</p>
Copy link
Member

Choose a reason for hiding this comment

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

Do we need both a div and a p, or would one or the other have been sufficient?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replaced with Placeholder within #6571.

getQueryUrlFromObject( obj, prefix ) {
return map( obj, ( paramValue, paramName ) => {
const key = prefix ? prefix + '[' + paramName + ']' : paramName,
value = obj[ paramName ];
Copy link
Member

Choose a reason for hiding this comment

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

Are paramValue and value the same?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571.

return map( obj, ( paramValue, paramName ) => {
const key = prefix ? prefix + '[' + paramName + ']' : paramName,
value = obj[ paramName ];
return isObject( paramValue ) ? this.getQueryUrlFromObject( value, key ) :
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not intentional:

n_ > _.isObject( [] )
true
n_ > _.isObject( function() {} )
true

See also: https://lodash.com/docs/4.17.10#isPlainObject

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571.

}

fetch( props ) {
this.setState( { response: null } );
Copy link
Member

Choose a reason for hiding this comment

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

We should check if this.state.response is already null here, as it will be when this function is called by componentDidMount and we'll wastefully cause an immediate re-render (setState always incurs a render unless prevented by shouldComponentUpdate).

Calling setState() in [componentDidMount] will trigger an extra rendering, but it will happen before the browser updates the screen. This guarantees that even though the render() will be called twice in this case, the user won’t see the intermediate state. Use this pattern with caution because it often causes performance issues. It can, however, be necessary for cases like modals and tooltips when you need to measure a DOM node before rendering something that depends on its size or position.

https://reactjs.org/docs/react-component.html#componentdidmount

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571.


const path = '/gutenberg/v1/block-renderer/' + block + '?context=edit&' + this.getQueryUrlFromObject( { attributes } );

return wp.apiRequest( { path: path } ).then( ( response ) => {
Copy link
Member

Choose a reason for hiding this comment

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

There is available a mapping to @wordpress/api-request as a dependency to avoid referencing the global here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571.


render() {
const response = this.state.response;
if ( ! response || ! response.length ) {
Copy link
Member

Choose a reason for hiding this comment

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

Why do we check response.length ? Is it desirable that if an empty response was received from the server, we show a loading indicator forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed within #6571, let me know if it looks OK now.

} );
}

getQueryUrlFromObject( obj, prefix ) {
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a wordpress/url package utility.

Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a wordpress/url package utility.

In fact, it could probably be built-in as the default behavior for the existing addQueryArgs function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addQueryArgs works well but not for nested attributes, thus the need for creating a custom method. Couldn't find an existing utility that would put together URL in case of nested params, if you happen to know any, happy to change it!

Copy link
Member

Choose a reason for hiding this comment

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

If the behavior is needed, it could be added to the existing function via pull request to the packages repository.

const response = this.state.response;
if ( ! response || ! response.length ) {
return (
<div key="loading" className="wp-block-embed is-loading">
Copy link
Member

Choose a reason for hiding this comment

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

We shouldn't reuse classes but components. If there's something in the way the embed blocks handles the loading state that is useful, it should be extracted in a separate UI component.

@jeffpaul
Copy link
Member

jeffpaul commented May 2, 2018

@mtias @aduth are your recent comments here something that should be added to a new issue that someone could help pickup and resolve? I want to make sure we capture those as otherwise nothing will happen with your input if we just leave them on this PR post-merge.

@aduth
Copy link
Member

aduth commented May 2, 2018

They could be, yes. My hope would be that the feedback wouldn't simply be ignored by the original participants though.

@jeffpaul
Copy link
Member

jeffpaul commented May 2, 2018

@aduth in my experience, many comments post-merge get lost. So let's capture actionable feedback and get that into a new issue so someone can pick up that work. Feel free to emoji-react on certain comments that are actionable and I can go through and pull all those into a new issue if you'd like.

@miina
Copy link
Contributor Author

miina commented May 2, 2018

@jeffpaul @aduth Looks like @westonruter found a potential bug with SSR component when using the Columns block. Perhaps the post-merge feedback could be addressed together with that.

@mtias
Copy link
Member

mtias commented May 3, 2018

Yes, I don't think an issue is needed in this particular case as the comments with the code are easier to follow — however you prefer, @miina.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. Framework Issues related to broader framework topics, especially as it relates to javascript Needs Testing Needs further testing to be confirmed. REST API Interaction Related to REST API
Projects
None yet
Development

Successfully merging this pull request may close these issues.