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

Fix using the classic block in nested contexts #16477

Merged
merged 3 commits into from
Jul 9, 2019

Conversation

youknowriad
Copy link
Contributor

At the root level, the classic block omits its delimiters in order to support posts written with alternative editors. In nested contexts though, the classic editor should be rendered with delimiters otherwise the parser won't be able to reparse the post properly.

closes #13187

Testing instructions

  • Use a classic block in a columns block
  • Save and refresh
  • Everything should work as intended.

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended Framework Issues related to broader framework topics, especially as it relates to javascript [Block] Classic Affects the Classic Editor Block [Component] Serializer labels Jul 9, 2019
@youknowriad youknowriad self-assigned this Jul 9, 2019
@@ -280,9 +281,12 @@ export function serializeBlock( block ) {
* Takes a block or set of blocks and returns the serialized post content.
*
* @param {Array} blocks Block(s) to serialize.
* @param {Object} options Serialization options.
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 document these options. I think the "best" way would to define a JSDoc typedef of something like WPBlockSerializationOptions with each property described. Unfortunately docgen won't yet output these, but it seems prime for a future enhancement.

*
* @return {string} Serialized block.
*/
export function serializeBlock( block ) {
export function serializeBlock( block, { isNested = false } = {} ) {
Copy link
Member

Choose a reason for hiding this comment

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

Nit: I think we should try to consolidate our vocabulary around "inner blocks". Do we have many other references of "nested" in code? I know there's a tendency to refer to them this way in casual conversation. Is isInnerBlock reasonable as an alternative here?

switch ( blockName ) {
case getFreeformContentHandlerName():
case getUnregisteredTypeHandlerName():
switch ( true ) {
Copy link
Member

Choose a reason for hiding this comment

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

At this point, I think we'd just not want to bother with a switch and use if instead. Alternatively, we could intentionally use fallthrough to allow the nested case to fall through to default.

switch ( blockName ) {
	case getUnregisteredTypeHandlerName():
		return saveContent;

	case getFreeformContentHandlerName():
		if ( ! isNested ) {
			return saveContent;
		}

	default: {
		const blockType = getBlockType( blockName );
		const saveAttributes = getCommentAttributes( blockType, block.attributes );
		return getCommentDelimitedContent( blockName, saveAttributes, saveContent );
	}
}

It's pretty confusing though. Maybe less-so if default was just code following the switch instead of a case, but still confusing I think.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

Pretty simple fix 👍

*
* @return {string} Serialized block.
*/
export function serializeBlock( block, { isNested = false } = {} ) {
export function serializeBlock( block, { isInnerBlocks = false } = {} ) {
Copy link
Member

Choose a reason for hiding this comment

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

Should be singular isInnerBlock?

Edit: Oh, I guess in the context that this is an option just passed along from serialize which is for multiple blocks, it makes sense as written, unless we wanted to rename the option as it applies to the context of the singular block being serialized. I think that comes with its own confusion. I'll leave it to you to judge, but I'm fine either way.

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 think I'd leave it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Classic Affects the Classic Editor Block [Feature] Saving Related to saving functionality Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding a classic block inside a columns block causes a block validation failure.
3 participants