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

Framework: Bootstrap Block Templates #3668

Merged
merged 2 commits into from
Nov 27, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Nov 27, 2017

This PR is the first one introducing the Block Templates. (details on those here #3588 )
This first iteration allows:

  • Defining a template for a CPT
  • Initialize the editor with a predefined list of blocks if a template is found.

A template object looks like this:

const template = [
  [ 'block/name', {} ], // [ blockName, attributes ]
];

Testing instructions

  • Add A CPT like this:
function register_book_type() {
	$args = array(
		'public' => true,
		'label'  => 'Books',
		'show_in_rest' => true,
		'template' => array(
			array( 'core/image' ),
			array( 'core/paragraph', array(
				'placeholder' => 'Add a book description',
			) ),
			array( 'core/quote' ),
		),
	);
	register_post_type( 'book', $args );
}
add_action( 'init', 'register_book_type' );
  • Create a new post with the registered CPT
  • The editor should initialize with the predefined list of blocks defined in this template.

Notes

  • Block Attributes need to be initialized properly server-side, we should move away from our Editable Value as React Elements to be able to do so for these attributes (related Editable: state as a cleaned up tree #3048)

@youknowriad youknowriad added the Framework Issues related to broader framework topics, especially as it relates to javascript label Nov 27, 2017
@youknowriad youknowriad self-assigned this Nov 27, 2017
@youknowriad youknowriad force-pushed the add/initialize-editor-with-template branch from 7a8ded7 to 64ab4db Compare November 27, 2017 11:49
@codecov
Copy link

codecov bot commented Nov 27, 2017

Codecov Report

Merging #3668 into master will decrease coverage by 0.01%.
The diff coverage is 20%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #3668      +/-   ##
=========================================
- Coverage   37.32%   37.3%   -0.02%     
=========================================
  Files         277     277              
  Lines        6685    6690       +5     
  Branches     1213    1214       +1     
=========================================
+ Hits         2495    2496       +1     
- Misses       3532    3536       +4     
  Partials      658     658
Impacted Files Coverage Δ
editor/actions.js 47.16% <ø> (ø) ⬆️
editor/components/provider/index.js 13.33% <0%> (+0.83%) ⬆️
editor/effects.js 58.06% <28.57%> (-2.11%) ⬇️

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 9864a60...8b125bb. Read the comment docs.

const effects = [];

// Parse content as blocks
if ( post.content.raw ) {
effects.push( resetBlocks( parse( post.content.raw ) ) );
} else if ( settings.template ) {
const blocks = map( settings.template.blocks, ( { name, attributes } ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure we are going to want additional properties on the template object instead of making template = template.blocks?

Copy link
Contributor Author

@youknowriad youknowriad Nov 27, 2017

Choose a reason for hiding this comment

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

I don't know honestly. Server-side I think so, because templates could have a name/description but client-side I don't really know. I thought we should do this to avoid future breaking changes.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, just thinking out-loud. There are things like "locking" which might be interesting there. Not sure about naming templates, but maybe an id / slug if they are stored somewhere?

Copy link
Member

@mtias mtias Nov 27, 2017

Choose a reason for hiding this comment

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

Thinking further, I can't see why we would pair defining blocks and naming in the CPT. If anything, the "template" would be defined outside the CPT and referenced here by slug.

Also, things like locking could be defined as another attribute of the CPT registration, which seems more in line with how we generally structure options:

'template' => array(
	array( 'core/image' ),
	array( 'core/paragraph', array(
		'placeholder' => 'Add a book description',
	) ),
	array( 'core/quote' ),
),
'template_lock' => true,

Finally, what do you think of simplifying the syntax to not require name => 'core/heading'? I think we can rely on the definition of a block to be array( blockName, 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.

Sure, I was not sure about the extra keys anyway.

};
return block;
} );
effects.push( resetBlocks( blocks ) );
Copy link
Member

Choose a reason for hiding this comment

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

I love that this fairly low profile implementation wise.

@youknowriad youknowriad force-pushed the add/initialize-editor-with-template branch from 64ab4db to 55454fe Compare November 27, 2017 13:04
@mtias
Copy link
Member

mtias commented Nov 27, 2017

This is looking good to me. We probably want to add a test and some documentation. I think we might need a new "Templates" document. (Doesn't have to block this.)

@youknowriad youknowriad merged commit deb51e1 into master Nov 27, 2017
@youknowriad youknowriad deleted the add/initialize-editor-with-template branch November 27, 2017 14:59
@youknowriad
Copy link
Contributor Author

Merged, let's add the docs and tests separately.

@@ -782,6 +782,11 @@ function gutenberg_editor_scripts_and_styles( $hook ) {
'blockTypes' => $allowed_block_types,
);

$post_type_object = get_post_type_object( $post_to_edit['type'] );
if ( $post_type_object->template ) {
Copy link
Member

@aduth aduth Nov 27, 2017

Choose a reason for hiding this comment

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

Notice: Undefined property: WP_Post_Type::$template in /srv/www/editor/htdocs/wp-content/plugins/gutenberg/lib/client-assets.php on line 786

I suggest running with high error reporting:

error_reporting( E_ALL );

(Add near top of site root index.php)

Copy link
Member

Choose a reason for hiding this comment

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

You want empty here:

if ( ! empty( $post_type_object->template ) ) {

@baerkins
Copy link

baerkins commented Dec 1, 2017

I'm not sure if this is intended, but omitting 'show_in_rest' => true, or setting it to false results in the default editor appearing.

@youknowriad
Copy link
Contributor Author

@iamhexcoder Yes, this is expected. Gutenberg uses the REST API and if the show_in_rest is false, Gutenberg is not able to fetch an save the CPT. This is a current limitation of the REST API.

We'll figure out a way to solve this. Tracked in #3066

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants