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

Blocks: Support block nesting (Add Columns block, InnerBlocks component) #3745

Merged
merged 13 commits into from
Feb 6, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
51 changes: 44 additions & 7 deletions blocks/api/factory.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { applyFilters } from '@wordpress/hooks';

/**
* Internal dependencies
Expand All @@ -32,10 +33,11 @@ import { getBlockType, getBlockTypes } from './registration';
*
* @param {string} name Block name.
* @param {Object} blockAttributes Block attributes.
* @param {?Array} innerBlocks Nested blocks.
*
* @return {Object} Block object.
*/
export function createBlock( name, blockAttributes = {} ) {
export function createBlock( name, blockAttributes = {}, innerBlocks = [] ) {
// Get the type definition associated with a registered block.
const blockType = getBlockType( name );

Expand All @@ -59,6 +61,29 @@ export function createBlock( name, blockAttributes = {} ) {
name,
isValid: true,
attributes,
innerBlocks,
};
}

/**
* Given a block object, returns a copy of the block object, optionally merging
* new attributes and/or replacing its inner blocks.
*
* @param {Object} block Block object.
* @param {Object} mergeAttributes Block attributes.
* @param {?Array} innerBlocks Nested blocks.
*
* @return {Object} A cloned block.
*/
export function cloneBlock( block, mergeAttributes = {}, innerBlocks = block.innerBlocks ) {
return {
...block,
uid: uuid(),
attributes: {
...block.attributes,
...mergeAttributes,
},
innerBlocks,
};
}

Expand Down Expand Up @@ -213,12 +238,24 @@ export function switchToBlockType( blocks, name ) {
return null;
}

return transformationResults.map( ( result, index ) => ( {
...result,
// The first transformed block whose type matches the "destination"
// type gets to keep the existing UID of the first block.
uid: index === firstSwitchedBlock ? firstBlock.uid : result.uid,
} ) );
return transformationResults.map( ( result, index ) => {
const transformedBlock = {
...result,
// The first transformed block whose type matches the "destination"
// type gets to keep the existing UID of the first block.
uid: index === firstSwitchedBlock ? firstBlock.uid : result.uid,
};

/**
* Filters an individual transform result from block transformation.
* All of the original blocks are passed, since transformations are
* many-to-many, not one-to-one.
*
* @param {Object} transformedBlock The transformed block.
* @param {Object[]} blocks Original blocks transformed.
*/
return applyFilters( 'blocks.switchToBlockType.transformedBlock', transformedBlock, blocks );
} );
}

/**
Expand Down
8 changes: 7 additions & 1 deletion blocks/api/index.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,10 @@
export { createBlock, getPossibleBlockTransformations, switchToBlockType, createReusableBlock } from './factory';
export {
createBlock,
cloneBlock,
getPossibleBlockTransformations,
switchToBlockType,
createReusableBlock,
} from './factory';
export { default as parse, getBlockAttributes } from './parser';
export { default as rawHandler } from './raw-handling';
export {
Expand Down
27 changes: 20 additions & 7 deletions blocks/api/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -178,13 +178,23 @@ export function getAttributesFromDeprecatedVersion( blockType, innerHTML, attrib
/**
* Creates a block with fallback to the unknown type handler.
*
* @param {?string} name Block type name.
* @param {string} innerHTML Raw block content.
* @param {?Object} attributes Attributes obtained from block delimiters.
* @param {Object} blockNode Parsed block node.
*
* @return {?Object} An initialized block object (if possible).
*/
export function createBlockWithFallback( name, innerHTML, attributes ) {
export function createBlockWithFallback( blockNode ) {
let {
blockName: name,
attrs: attributes,
innerBlocks = [],
innerHTML,
} = blockNode;

attributes = attributes || {};

// Trim content to avoid creation of intermediary freeform segments
innerHTML = innerHTML.trim();
Copy link
Member

Choose a reason for hiding this comment

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

How was it being created otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

I see it was already present just in a different moment.


// Use type from block content, otherwise find unknown handler.
name = name || getUnknownTypeHandlerName();

Expand Down Expand Up @@ -217,14 +227,18 @@ export function createBlockWithFallback( name, innerHTML, attributes ) {
blockType = getBlockType( name );
}

// Coerce inner blocks from parse form to canonical form
innerBlocks = innerBlocks.map( createBlockWithFallback );

// Include in set only if type were determined.
if ( ! blockType || ( ! innerHTML && name === fallbackBlock ) ) {
return;
}

const block = createBlock(
name,
getBlockAttributes( blockType, innerHTML, attributes )
getBlockAttributes( blockType, innerHTML, attributes ),
innerBlocks
);

// Validate that the parsed block is valid, meaning that if we were to
Expand Down Expand Up @@ -264,8 +278,7 @@ export function createBlockWithFallback( name, innerHTML, attributes ) {
*/
export function parseWithGrammar( content ) {
return grammarParse( content ).reduce( ( memo, blockNode ) => {
const { blockName, innerHTML, attrs } = blockNode;
const block = createBlockWithFallback( blockName, innerHTML.trim(), attrs );
const block = createBlockWithFallback( blockNode );
if ( block ) {
memo.push( block );
}
Expand Down
38 changes: 25 additions & 13 deletions blocks/api/serializer.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { hasFilter, applyFilters } from '@wordpress/hooks';
* Internal dependencies
*/
import { getBlockType, getUnknownTypeHandlerName } from './registration';
import BlockContentProvider from '../block-content-provider';

/**
* Returns the block's default classname from its name.
Expand All @@ -32,12 +33,13 @@ export function getBlockDefaultClassname( blockName ) {
* Given a block type containg a save render implementation and attributes, returns the
* enhanced element to be saved or string when raw HTML expected.
*
* @param {Object} blockType Block type.
* @param {Object} attributes Block attributes.
* @param {Object} blockType Block type.
* @param {Object} attributes Block attributes.
* @param {?Array} innerBlocks Nested blocks.
*
* @return {Object|string} Save content.
* @return {Object|string} Save element or raw HTML string.
*/
export function getSaveElement( blockType, attributes ) {
export function getSaveElement( blockType, attributes, innerBlocks = [] ) {
let { save } = blockType;

// Component classes are unsupported for save since serialization must
Expand All @@ -48,7 +50,7 @@ export function getSaveElement( blockType, attributes ) {
save = instance.render.bind( instance );
}

let element = save( { attributes } );
let element = save( { attributes, innerBlocks } );

if ( isObject( element ) && hasFilter( 'blocks.getSaveContent.extraProps' ) ) {
/**
Expand Down Expand Up @@ -77,20 +79,27 @@ export function getSaveElement( blockType, attributes ) {
* @param {WPBlockType} blockType Block type definition.
* @param {Object} attributes Block attributes.
*/
return applyFilters( 'blocks.getSaveElement', element, blockType, attributes );
element = applyFilters( 'blocks.getSaveElement', element, blockType, attributes );

return (
<BlockContentProvider innerBlocks={ innerBlocks }>
{ element }
</BlockContentProvider>
);
}

/**
* Given a block type containg a save render implementation and attributes, returns the
* static markup to be saved.
*
* @param {Object} blockType Block type.
* @param {Object} attributes Block attributes.
* @param {Object} blockType Block type.
* @param {Object} attributes Block attributes.
* @param {?Array} innerBlocks Nested blocks.
*
* @return {string} Save content.
*/
export function getSaveContent( blockType, attributes ) {
return renderToString( getSaveElement( blockType, attributes ) );
export function getSaveContent( blockType, attributes, innerBlocks ) {
return renderToString( getSaveElement( blockType, attributes, innerBlocks ) );
}

/**
Expand Down Expand Up @@ -171,11 +180,14 @@ export function getBlockContent( block ) {
const blockType = getBlockType( block.name );

// If block was parsed as invalid or encounters an error while generating
// save content, use original content instead to avoid content loss.
// save content, use original content instead to avoid content loss. If a
// block contains nested content, exempt it from this condition because we
// otherwise have no access to its original content and content loss would
// still occur.
let saveContent = block.originalContent;
if ( block.isValid ) {
if ( block.isValid || block.innerBlocks.length ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

So, If I understand properly we're ignoring the validation because we can't validate properly the container blocks? Can you explain a bit more why we can't validate the container blocks properly? (I'm certain you already did elsewhere, just a link maybe :))

Why are we creating a difference between blocks with inner blocks and blocks without inner blocks. If we can't guarantee the behavior for all blocks, shouldn't we just remove the condition to make it consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

The main issue is that if we mark the block as invalid, it will defer the next save to using its innerHTML; this is usually meant to preserve the original content, but in a case of a block which contains nested blocks, it destroys the content, because nested content is not reflected in a block's innerHTML.

Copy link
Member

Choose a reason for hiding this comment

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

Also, that we'd be validating the same content n-times.

try {
saveContent = getSaveContent( blockType, block.attributes );
saveContent = getSaveContent( blockType, block.attributes, block.innerBlocks );
} catch ( error ) {}
}

Expand Down
94 changes: 89 additions & 5 deletions blocks/api/test/factory.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,19 @@
/**
* External dependencies
*/
import deepFreeze from 'deep-freeze';
import { noop } from 'lodash';

/**
* Internal dependencies
*/
import { createBlock, getPossibleBlockTransformations, switchToBlockType, createReusableBlock } from '../factory';
import {
createBlock,
cloneBlock,
getPossibleBlockTransformations,
switchToBlockType,
createReusableBlock,
} from '../factory';
import { getBlockTypes, unregisterBlockType, setUnknownTypeHandlerName, registerBlockType } from '../registration';

describe( 'block factory', () => {
Expand Down Expand Up @@ -34,7 +41,7 @@ describe( 'block factory', () => {
} );

describe( 'createBlock()', () => {
it( 'should create a block given its blockType and attributes', () => {
it( 'should create a block given its blockType, attributes, inner blocks', () => {
registerBlockType( 'core/test-block', {
attributes: {
align: {
Expand All @@ -53,9 +60,11 @@ describe( 'block factory', () => {
category: 'common',
title: 'test block',
} );
const block = createBlock( 'core/test-block', {
align: 'left',
} );
const block = createBlock(
'core/test-block',
{ align: 'left' },
[ createBlock( 'core/test-block' ) ],
);

expect( block.name ).toEqual( 'core/test-block' );
expect( block.attributes ).toEqual( {
Expand All @@ -64,6 +73,8 @@ describe( 'block factory', () => {
align: 'left',
} );
expect( block.isValid ).toBe( true );
expect( block.innerBlocks ).toHaveLength( 1 );
expect( block.innerBlocks[ 0 ].name ).toBe( 'core/test-block' );
expect( typeof block.uid ).toBe( 'string' );
} );

Expand Down Expand Up @@ -103,6 +114,79 @@ describe( 'block factory', () => {
} );
} );

describe( 'cloneBlock()', () => {
it( 'should merge attributes into the existing block', () => {
registerBlockType( 'core/test-block', {
attributes: {
align: {
type: 'string',
},
isDifferent: {
type: 'boolean',
default: false,
},
},
save: noop,
category: 'common',
title: 'test block',
} );
const block = deepFreeze(
createBlock(
'core/test-block',
{ align: 'left' },
[ createBlock( 'core/test-block' ) ],
)
);

const clonedBlock = cloneBlock( block, {
isDifferent: true,
} );

expect( clonedBlock.name ).toEqual( block.name );
expect( clonedBlock.attributes ).toEqual( {
align: 'left',
isDifferent: true,
} );
expect( clonedBlock.innerBlocks ).toHaveLength( 1 );
expect( typeof clonedBlock.uid ).toBe( 'string' );
expect( clonedBlock.uid ).not.toBe( block.uid );
} );

it( 'should replace inner blocks of the existing block', () => {
registerBlockType( 'core/test-block', {
attributes: {
align: {
type: 'string',
},
isDifferent: {
type: 'boolean',
default: false,
},
},
save: noop,
category: 'common',
title: 'test block',
} );
const block = deepFreeze(
createBlock(
'core/test-block',
{ align: 'left' },
[
createBlock( 'core/test-block', { align: 'right' } ),
createBlock( 'core/test-block', { align: 'left' } ),
],
)
);

const clonedBlock = cloneBlock( block, undefined, [
createBlock( 'core/test-block' ),
] );

expect( clonedBlock.innerBlocks ).toHaveLength( 1 );
expect( clonedBlock.innerBlocks[ 0 ].attributes ).not.toHaveProperty( 'align' );
} );
} );

describe( 'getPossibleBlockTransformations()', () => {
it( 'should should show as available a simple "from" transformation"', () => {
registerBlockType( 'core/updated-text-block', {
Expand Down
Loading