Skip to content

Conversation

@jorgefilipecosta
Copy link
Member

Until now when templates set HTML in attributes that supported it e.g: the content of a paragraph, the HTML was ignored/escaped. Now we correctly parse the HTML set in the template.
This allows more advanced templates to be created.

How has this been tested?

Add CPT that sets a template with HTML code and verify things work as expected.
Sample code:

function myplugin_register_book_post_typeTMP() {
	$args = array(
		'public' => true,
		'label'  => 'BooksTMP',
		'show_in_rest' => true,
		'template' => array(
			array( 'core/list', array(
				'values' => '<li>L1</li><li>L2</li>',
			) ),
			array( 'core/paragraph', array(
				'placeholder' => 'Add Description...',
			) ),
			array( 'core/paragraph', array(
				'placeholder' => 'Add Description...',
				'content' => 'simple text'
			) ),
			array( 'core/paragraph', array(
				'content' => '<b>Single Child</b>'
			) ),
			array( 'core/paragraph', array(
				'content' => 'text <b>bold</b> <a href="http//www.example.com">link</a>',
			) ),
			array(
				'core/columns',
				array(
					'columns' => 2,
				),
				array(
					array(
						'core/paragraph',
						array(
							'content' => 'nested <b>bold</b> <a href="http//www.example.com">link</a>',
							'layout' => 'column-1',
						)
					)
				)
			)
		),
	);
	register_post_type( 'book-tmp', $args );
}
add_action( 'init', 'myplugin_register_book_post_typeTMP' );
```

@jorgefilipecosta jorgefilipecosta added the [Type] Enhancement A suggestion for improvement. label May 25, 2018
@jorgefilipecosta jorgefilipecosta self-assigned this May 25, 2018

const blockType = getBlockType( name );
const attributesParsed = mapValues( attributes, ( attributeValue, attributeName ) => {
if ( isString( attributeValue ) && 'array' === get( blockType, [ 'attributes', attributeName, 'type' ] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I personally think this is fragile, I don't like special cases like this, I personally prefer if we update this globally: Use "string" for the RichText format by default to avoid the children source entirely or come up with a simple tree structure as an alternative to the React element structure.

There has been a lot of back and forth about the RichText default format, I think we should make a decision and move forward once and for all. cc @aduth @iseulde @mtias

Copy link
Contributor

Choose a reason for hiding this comment

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

There has been a lot of back and forth about the RichText default format, I think we should make a decision and move forward once and for all.

I agree we need some decision here, but the memory also needs a refreshing on the trade-offs.

The other thing is: regardless of that decision, should the possibilities offered to developers hinge on blocks' particular choices of rich-text format? I shouldn't think so. That is, it is a poor experience if I can provide a full template for core/foo but not for core/bar because the latter uses type: 'array, even if this were no longer the default type.

Copy link
Contributor

@youknowriad youknowriad May 26, 2018

Choose a reason for hiding this comment

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

I think if we want to offer an alternative template mechanism, I'd like to avoid applying a random matcher "children" based on the fact that the type is "array" while we have a "string". This could be an error, "children" is not the only matcher producing an array, "query" does too.

I'd prefer if we consider using a string for the whole block and not only one attribute. Instead of doing this, this is less ambiguous for me:

template = [
  [ 'core/block', { attr1: [ 'value' ], attr2: [ 'blabla' ] } ],
]

I'd do

template = [
  '<!-- wp:core/block { attr1: value } --><p>blabla</p><! /wp:core/block -->'
]

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 agree that a string looks like a good idea. I would go far and not even have an array for the template just a single string (that themes could read from an HTML file).
Even if we set an array of blocks, one block may have other blocks inside, so it looks a single string is more intuitive.
The only downside is if later we need some flag that templates can set when creating the blocks, the current approach we have can easily allow that and the new method does not allow that as easily.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, the string should be a fallback, maybe per block by I'm not certain. I'm thinking about locking. Right now we can lock globally but it's planned to be able to lock per block and a single string won't allow that. while the array notation does.

But no strong opinion personally.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, the string should be a fallback, maybe per block by I'm not certain. I'm thinking about locking. Right now we can lock globally but it's planned to be able to lock per block and a single string won't allow that. while the array notation does.

Even if we use an array, we would not be able to lock a paragraph inside columns because all the content inside columns would be a single string so for nested blocks we would not have an array.
Right now for placeholders, we use an attribute, even though that attribute is only used in the editor context. Maybe we can do the same for locking. We can have a boolean attribute that enables locking. That attribute would be added to all the blocks via a hook; blocks can opt-out via supports.

Copy link
Member

@mtias mtias May 28, 2018

Choose a reason for hiding this comment

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

I don't like:

template = [
  '<!-- wp:core/block { attr1: value } --><p>blabla</p><! /wp:core/block -->'
]

Because it leaks an implementation detail that should be opaque to the block developer. (The HTML 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.

Because it leaks an implementation detail that should be opaque to the block developer. (The HTML comment.)

Well remembered @mtias.

Without using the serialized content as the template the only option besides this one is to Change our data structure for children to something serialized and that can be easily built with arrays and objects.

I think if we want to offer an alternative template mechanism, I'd like to avoid applying a random matcher "children" based on the fact that the type is "array" while we have a "string". This could be an error, "children" is not the only matcher producing an array, "query" does too.

Hi @youknowriad, for the query matcher the template should not set a string but an array itself with an object with the result of the matching. I think the only case where template sets a string type and the attribute declares an array is this one. This one is also the only case that needs parsing as in all the other cases the template sets simple types (bool, number, string) or arrays of simple types. But in order to make the logic more explicit, I added a check for the children source.

This is not the only case in the code where we convert a string to the "children" structure, it happens also for example when parsing the HTML from image captions. In this last case even if we change our data structure we will need to do parsing to our data structure like we do know because the source will always be a string.

Until know when templates set HTML in attributes that supported it e.g: the content of a paragraph, the HTML was ignored/escaped. Now we correctly parse the HTML set in the template.
This allows more advanced templates to be created.
@jorgefilipecosta jorgefilipecosta force-pushed the add/template-html-attributes branch from c9bf331 to b21bc26 Compare May 28, 2018 19:01
@aduth
Copy link
Member

aduth commented May 29, 2018

I don't think we need to support this. Is there prior discussion where this was rationalized?

There has been a lot of back and forth about the RichText default format, I think we should make a decision and move forward once and for all.

Related: #5380 My most recent and exhaustive overview of the problem.

avoid the children source entirely or come up with a simple tree structure as an alternative to the React element structure.

I'm personally not opposed to some minimal simplification of an "Element" type, which is an object containing type (string) and props (object). We just need to decide how far we intend to support additional features of React, like specially handled props (children, style, dangerouslySetInnerHTML, key, etc).

Related prior art:

With this mindset, I'm not sensing as much urgency to abandon the React tree children type, if we both limit (or abstract) our direct inspection/manipulation of this value type, or at least in where we can settle on some well-knowns (a type, or a children array) that won't ever change, even in the worst case where we move away from React.

@mcsf
Copy link
Contributor

mcsf commented May 30, 2018

For what it's worth, this works out of the box (extending the server-side template at test/e2e/test-plugins/templates.php):

screen shot 2018-05-30 at 17 50 38

I'd say that this faking of element trees is probably enough for most use cases where authors want to define certain rich-text content in templates. Further, I don't believe core needs to support this transformation itself, at least not right away; in other words, an author knows best how to provide fake element trees to the template, whether manually or via their own toFakeTree( html ) function.

@jorgefilipecosta
Copy link
Member Author

Thank you all for the discussion. I'm going to close this PR as it looks like @mcsf found a workaround that allows HTML in templates.
cc: @nosolosw

@jorgefilipecosta jorgefilipecosta deleted the add/template-html-attributes branch May 30, 2018 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants