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

Render generic fallback block for unknown block type #335

Merged
merged 9 commits into from
Mar 31, 2017
5 changes: 3 additions & 2 deletions .eslintrc.json
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@
"one-var": "off",
"padded-blocks": [ "error", "never" ],
"quote-props": [ "error", "as-needed", { "keywords": true } ],
"react/prop-types": "off",
"react/display-name": "off",
"semi": "error",
"semi-spacing": "error",
"space-before-blocks": [ "error", "always" ],
Expand All @@ -74,7 +76,6 @@
"!": true
}
} ],
"valid-jsdoc": [ "error", { "requireReturn": false } ],
"react/prop-types": "off"
"valid-jsdoc": [ "error", { "requireReturn": false } ]
}
}
9 changes: 8 additions & 1 deletion blocks/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,4 +7,11 @@ export { query };
export { default as Editable } from './components/editable';
export { default as parse } from './parser';
export { getCategories } from './categories';
export { registerBlock, unregisterBlock, getBlockSettings, getBlocks } from './registration';
export {
registerBlock,
unregisterBlock,
setUnknownTypeHandler,
getUnknownTypeHandler,
getBlockSettings,
getBlocks
} from './registration';
32 changes: 22 additions & 10 deletions blocks/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,24 +7,26 @@ import * as query from 'hpq';
* Internal dependencies
*/
import { parse as grammarParse } from './post.pegjs';
import { getBlockSettings } from './registration';
import { getBlockSettings, getUnknownTypeHandler } from './registration';

/**
* Returns the block attributes of a registered block node given its settings.
*
* @param {Object} blockNode Parsed block node
* @param {Object} blockSettings Block settings
* @return {Object} Block state, or undefined if type unknown
* @param {Object} blockNode Parsed block node
* @param {Object} blockSettings Block settings
* @return {Object} Block attributes
*/
export function getBlockAttributes( blockNode, blockSettings ) {
const { rawContent } = blockNode;

// Merge attributes from parse with block implementation
let { attrs } = blockNode;
if ( 'function' === typeof blockSettings.attributes ) {
attrs = { ...attrs, ...blockSettings.attributes( rawContent ) };
} else if ( blockSettings.attributes ) {
attrs = { ...attrs, ...query.parse( rawContent, blockSettings.attributes ) };
if ( blockSettings ) {
if ( 'function' === typeof blockSettings.attributes ) {
attrs = { ...attrs, ...blockSettings.attributes( rawContent ) };
} else if ( blockSettings.attributes ) {
attrs = { ...attrs, ...query.parse( rawContent, blockSettings.attributes ) };
}
}

return attrs;
Expand All @@ -38,11 +40,21 @@ export function getBlockAttributes( blockNode, blockSettings ) {
*/
export default function parse( content ) {
return grammarParse( content ).reduce( ( memo, blockNode ) => {
const settings = getBlockSettings( blockNode.blockType );
// Use type from block node, otherwise find unknown handler
let { blockType = getUnknownTypeHandler() } = blockNode;

// Try finding settings for known block type, else again fall back
let settings = getBlockSettings( blockType );
if ( ! settings ) {
blockType = getUnknownTypeHandler();
settings = getBlockSettings( blockType );
}

// Include in set only if settings were determined
if ( settings ) {
memo.push( {
blockType: blockNode.blockType,
blockType,
rawContent: blockNode.rawContent,
attributes: getBlockAttributes( blockNode, settings )
} );
}
Expand Down
1 change: 0 additions & 1 deletion blocks/post.pegjs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ WP_Block_Html
= ts:(!WP_Block_Balanced c:Any { return c })+
{
return {
blockType: 'html',
attrs: {},
rawContent: ts.join( '' )
}
Expand Down
28 changes: 27 additions & 1 deletion blocks/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,17 @@
/**
* Block settings keyed by block slug.
*
* @var {Object} blocks
* @type {Object}
*/
const blocks = {};

/**
* Slug of block handling unknown types.
*
* @type {?string}
*/
let unknownTypeHandler;

/**
* Registers a new block provided a unique slug and an object defining its
* behavior. Once registered, the block is made available as an option to any
Expand Down Expand Up @@ -60,6 +67,25 @@ export function unregisterBlock( slug ) {
return oldBlock;
}

/**
* Assigns slug of block handling unknown block types.
*
* @param {string} slug Block slug
*/
export function setUnknownTypeHandler( slug ) {
unknownTypeHandler = slug;
}

/**
* Retrieves slug of block handling unknown block types, or undefined if no
* handler has been defined.
*
* @return {?string} Blog slug
*/
export function getUnknownTypeHandler() {
return unknownTypeHandler;
}

/**
* Returns settings associated with a registered block.
*
Expand Down
65 changes: 54 additions & 11 deletions blocks/test/parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,11 @@ import { text } from 'hpq';
* Internal dependencies
*/
import { default as parse, getBlockAttributes } from '../parser';
import { getBlocks, unregisterBlock, registerBlock } from '../registration';
import { getBlocks, unregisterBlock, setUnknownTypeHandler, registerBlock } from '../registration';

describe( 'block parser', () => {
beforeEach( () => {
afterEach( () => {
setUnknownTypeHandler( undefined );
getBlocks().forEach( ( block ) => {
unregisterBlock( block.slug );
} );
Expand Down Expand Up @@ -50,36 +51,78 @@ describe( 'block parser', () => {

const blockNode = {
blockType: 'core/test-block',
attrs: {},
attrs: {
align: 'left'
},
rawContent: '<span>Ribs <strong>& Chicken</strong></span>'
};

expect( getBlockAttributes( blockNode, blockSettings ) ).to.eql( {
align: 'left',
emphasis: '& Chicken'
} );
} );

it( 'should return parsed attributes for block without attributes defined', () => {
const blockSettings = {};

const blockNode = {
blockType: 'core/test-block',
attrs: {
align: 'left'
},
rawContent: '<span>Ribs <strong>& Chicken</strong></span>'
};

expect( getBlockAttributes( blockNode, blockSettings ) ).to.eql( {
align: 'left'
} );
} );
} );

describe( 'parse()', () => {
it( 'should parse the post content properly and ignore unknown blocks', () => {
const blockSettings = {
it( 'should parse the post content, ignoring unknown blocks', () => {
registerBlock( 'core/test-block', {
attributes: function( rawContent ) {
return {
content: rawContent + ' & Chicken'
};
}
};
registerBlock( 'core/test-block', blockSettings );
} );

const postContent = '<!-- wp:core/test-block -->Ribs<!-- /wp:core/test-block -->' +
'<!-- wp:core/unknown-block -->Ribs<!-- /wp:core/unknown-block -->';
const parsed = parse(
'<!-- wp:core/test-block -->Ribs<!-- /wp:core/test-block -->' +
'<p>Broccoli</p>' +
'<!-- wp:core/unknown-block -->Ribs<!-- /wp:core/unknown-block -->'
);

expect( parse( postContent ) ).to.eql( [ {
expect( parsed ).to.eql( [ {
blockType: 'core/test-block',
attributes: {
content: 'Ribs & Chicken'
}
},
rawContent: 'Ribs'
} ] );
} );

it( 'should parse the post content, using unknown block handler', () => {
registerBlock( 'core/test-block', {} );
registerBlock( 'core/unknown-block', {} );

setUnknownTypeHandler( 'core/unknown-block' );

const parsed = parse(
'<!-- wp:core/test-block -->Ribs<!-- /wp:core/test-block -->' +
'<p>Broccoli</p>' +
'<!-- wp:core/unknown-block -->Ribs<!-- /wp:core/unknown-block -->'
);

expect( parsed ).to.have.lengthOf( 3 );
expect( parsed.map( ( { blockType } ) => blockType ) ).to.eql( [
'core/test-block',
'core/unknown-block',
'core/unknown-block'
] );
} );
} );
} );
30 changes: 26 additions & 4 deletions blocks/test/registration.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,18 +9,26 @@ import sinon from 'sinon';
/**
* Internal dependencies
*/
import { getBlocks, unregisterBlock, registerBlock, getBlockSettings } from '../registration';
import {
registerBlock,
unregisterBlock,
setUnknownTypeHandler,
getUnknownTypeHandler,
getBlockSettings,
getBlocks
} from '../registration';

describe( 'blocks', () => {
// Reset block state before each test.
beforeEach( () => {
getBlocks().forEach( block => {
unregisterBlock( block.slug );
} );
sinon.stub( console, 'error' );
} );

afterEach( () => {
getBlocks().forEach( block => {
unregisterBlock( block.slug );
} );
setUnknownTypeHandler( undefined );
console.error.restore();
} );

Expand Down Expand Up @@ -86,6 +94,20 @@ describe( 'blocks', () => {
} );
} );

describe( 'setUnknownTypeHandler()', () => {
it( 'assigns unknown type handler', () => {
setUnknownTypeHandler( 'core/test-block' );

expect( getUnknownTypeHandler() ).to.equal( 'core/test-block' );
} );
} );

describe( 'getUnknownTypeHandler()', () => {
it( 'defaults to undefined', () => {
expect( getUnknownTypeHandler() ).to.be.undefined();
} );
} );

describe( 'getBlockSettings()', () => {
it( 'should return { slug } for blocks with no settings', () => {
registerBlock( 'core/test-block' );
Expand Down
23 changes: 23 additions & 0 deletions editor/blocks/freeform/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
const { html } = wp.blocks.query;

wp.blocks.registerBlock( 'core/freeform', {
title: 'Freeform',

icon: 'text',

category: 'common',

attributes: {
html: html()
},

edit( { attributes } ) {
return <div contentEditable>{ attributes.html }</div>;
},

save( { attributes } ) {
return attributes.html;
}
} );

wp.blocks.setUnknownTypeHandler( 'core/freeform' );
1 change: 1 addition & 0 deletions editor/blocks/index.js
Original file line number Diff line number Diff line change
@@ -1 +1,2 @@
import './freeform';
import './text';
4 changes: 2 additions & 2 deletions editor/blocks/text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ wp.blocks.registerBlock( 'core/text', {
value: html()
},

edit( attributes, onChange ) {
edit( { attributes, onChange } ) {
return (
<Editable
value={ attributes.value }
Expand All @@ -19,7 +19,7 @@ wp.blocks.registerBlock( 'core/text', {
);
},

save( attributes ) {
save( { attributes } ) {
return <p>{ attributes.value }</p>;
}
} );
26 changes: 19 additions & 7 deletions editor/editor/mode/visual.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,25 @@ function Blocks( { blocks, onChange } ) {

return (
<div className="editor-mode-visual">
<div>
{ blocks.map( ( block, index ) =>
<div key={ index }>
{ wp.blocks.getBlockSettings( block.blockType ).edit( block.attributes, onChangeBlock( index ) ) }
</div>
) }
Copy link
Contributor

Choose a reason for hiding this comment

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

If we fallback to a different blockType as suggested above, all the changes here are not needed anymore.

Copy link
Member Author

Choose a reason for hiding this comment

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

If we fallback to a different blockType as suggested above, all the changes here are not needed anymore.

I kept some changes mostly for consideration of:

  • What if a block decides it doesn't need to be editable, and only assigns a save implementation? Should we support this? With updated logic it will simply show the front-end UI within the editor.
  • We currently allow a block to register itself with neither edit nor save implementations, which will throw if left unchecked

Copy link
Contributor

Choose a reason for hiding this comment

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

and only assigns a save implementation?

Yep, could be good for separators and stuff like that.

We currently allow a block to register itself with neither edit nor save implementations, which will throw if left unchecked

I think we should add some validation to the registerBlock (category, save, title, icon are all mandatory to me) (but granted this is a separate task)

</div>
{ blocks.map( ( block, index ) => {
const settings = wp.blocks.getBlockSettings( block.blockType );

let BlockEdit;
if ( settings ) {
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 we can assume this is always defined, right!

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 think we can assume this is always defined, right!

I guess this depends if the editor implementation should trust itself to know that elsewhere it has assigned the unknown type handler. 😄 I might be erring too far on the side of safety here. Would certainly be more convenient to be able to assume that settings is always truthy.

BlockEdit = settings.edit || settings.save;
}

if ( ! BlockEdit ) {
return;
}

return (
<BlockEdit
key={ index }
attributes={ block.attributes }
onChange={ onChangeBlock( index ) } />
);
} ) }
<InserterButton />
</div>
);
Expand Down