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.

Sign up for GitHub

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

Block API: Introduce block definitions and implementations #6733

Closed
wants to merge 6 commits into from
Closed

Block API: Introduce block definitions and implementations #6733

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
036e05a
Blocks: Introduce block definitions and implementations
youknowriad May 14, 2018
a691fc7
Use the new API for the video block
youknowriad May 14, 2018
feb0600
Clarify implementation only attributes
youknowriad May 17, 2018
3d1fd0c
Small changes per review
youknowriad May 17, 2018
44ab993
Fix unit tests
youknowriad May 17, 2018
c55f8d9
Fix unit tests
youknowriad Jun 21, 2018
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
17 changes: 15 additions & 2 deletions blocks/api/registration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
/**
* External dependencies
*/
import { get, set, isFunction, some } from 'lodash';
import { get, set, isFunction, some, omit, mapValues, pick } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -168,7 +168,20 @@ export function registerBlockType( name, settings ) {
set( settings, [ 'supports', 'multiple' ], ! settings.useOnce );
}

dispatch( 'core/blocks' ).addBlockTypes( settings );
const implementationOnlyAttributes = [ 'transforms', 'edit', 'save', 'icon', 'getEditWrapperProps' ];
Copy link
Member

@gziolo gziolo Jun 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

deprecated is missing in here. Is it intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it's not. I intentionally avoided any deprecations for now. The idea is to do it only once, when we have all the bits in (the server-side part essentially)

Copy link
Member

Choose a reason for hiding this comment

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

I mean, it doesn't matter in this PR, but definitely something, we should keep in mind. Should we add todo comment?

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 it's not necessary because that's the first thing we should do when we move registration to the server. Tell people to register their blocks on the server.

const blockTypeDefinition = omit( settings, implementationOnlyAttributes );
blockTypeDefinition.attributes = mapValues(
settings.attributes,
( attribute ) => pick( attribute, [ 'type', 'default' ] )
);
const blockTypeImplementation = pick( settings, [ 'name' ].concat( implementationOnlyAttributes ) );
blockTypeImplementation.attributes = mapValues(
settings.attributes,
( attribute ) => omit( attribute, [ 'type', 'default' ] )
);

dispatch( 'core/blocks' ).addBlockTypes( blockTypeDefinition );
dispatch( 'core/blocks' ).implementBlockTypes( blockTypeImplementation );

return settings;
}
Expand Down
11 changes: 11 additions & 0 deletions blocks/api/test/registration.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,7 @@ describe( 'blocks', () => {
fill="red" stroke="blue" strokeWidth="10" />
</svg> ),
},
attributes: {},
} );
} );

Expand All @@ -233,6 +234,7 @@ describe( 'blocks', () => {
icon: {
src: 'foo',
},
attributes: {},
} );
} );

Expand All @@ -258,6 +260,7 @@ describe( 'blocks', () => {
icon: {
src: MyTestIcon,
},
attributes: {},
} );
} );

Expand Down Expand Up @@ -289,6 +292,7 @@ describe( 'blocks', () => {
fill="red" stroke="blue" strokeWidth="10" />
</svg> ),
},
attributes: {},
} );
} );

Expand All @@ -305,6 +309,7 @@ describe( 'blocks', () => {
icon: {
src: 'block-default',
},
attributes: {},
} );
} );

Expand Down Expand Up @@ -345,6 +350,7 @@ describe( 'blocks', () => {
icon: {
src: 'block-default',
},
attributes: {},
},
] );
const oldBlock = unregisterBlockType( 'core/test-block' );
Expand All @@ -357,6 +363,7 @@ describe( 'blocks', () => {
icon: {
src: 'block-default',
},
attributes: {},
} );
expect( getBlockTypes() ).toEqual( [] );
} );
Expand Down Expand Up @@ -401,6 +408,7 @@ describe( 'blocks', () => {
icon: {
src: 'block-default',
},
attributes: {},
} );
} );

Expand All @@ -416,6 +424,7 @@ describe( 'blocks', () => {
icon: {
src: 'block-default',
},
attributes: {},
} );
} );
} );
Expand All @@ -438,6 +447,7 @@ describe( 'blocks', () => {
icon: {
src: 'block-default',
},
attributes: {},
},
{
name: 'core/test-block-with-settings',
Expand All @@ -448,6 +458,7 @@ describe( 'blocks', () => {
icon: {
src: 'block-default',
},
attributes: {},
},
] );
} );
Expand Down
14 changes: 14 additions & 0 deletions blocks/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,20 @@ export function addBlockTypes( blockTypes ) {
};
}

/**
* Returns an action object used in signalling that block types have been implemented.
*
* @param {Array|Object} implementations Block types implementations.
*
* @return {Object} Action object.
*/
export function implementBlockTypes( implementations ) {
return {
type: 'IMPLEMENT_BLOCK_TYPES',
implementations: castArray( implementations ),
};
}

/**
* Returns an action object used to remove a registered block type.
*
Expand Down
23 changes: 23 additions & 0 deletions blocks/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,28 @@ export function blockTypes( state = {}, action ) {
return state;
}

/**
* Reducer managing the block type implementations
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function implementations( state = {}, action ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't necessarily oppose, but to me it was unexpected to see this as a separate branch on the client. Does the client really care about which bits of information are part of the definition and which of the implementation? As long as we have distinct actions, we can treat the bits differently as they come in if need be, but then we store everything in a common place for a single block type.

switch ( action.type ) {
case 'IMPLEMENT_BLOCK_TYPES':
return {
...state,
...keyBy( action.implementations, 'name' ),
};
case 'REMOVE_BLOCK_TYPES':
return omit( state, action.names );
}

return state;
}

/**
* Higher-order Reducer creating a reducer keeping track of given block name.
*
Expand Down Expand Up @@ -91,6 +113,7 @@ export function categories( state = DEFAULT_CATEGORIES, action ) {

export default combineReducers( {
blockTypes,
implementations,
defaultBlockName,
fallbackBlockName,
categories,
Expand Down
34 changes: 29 additions & 5 deletions blocks/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import createSelector from 'rememo';
import { filter, includes, map } from 'lodash';
import { filter, includes, map, mapValues, compact, get } from 'lodash';

/**
* Returns all the available block types.
Expand All @@ -12,9 +12,10 @@ import { filter, includes, map } from 'lodash';
* @return {Array} Block Types.
*/
export const getBlockTypes = createSelector(
( state ) => Object.values( state.blockTypes ),
( state ) => compact( Object.values( state.blockTypes ).map( ( { name } ) => getBlockType( state, name ) ) ),
( state ) => [
state.blockTypes,
state.implementations,
]
);

Expand All @@ -26,9 +27,32 @@ export const getBlockTypes = createSelector(
*
* @return {Object?} Block Type.
*/
export function getBlockType( state, name ) {
return state.blockTypes[ name ];
}
export const getBlockType = createSelector(
( state, name ) => {
const blockTypeDefinition = state.blockTypes[ name ];
const blockTypeImplementation = state.implementations[ name ];

if ( ! blockTypeDefinition || ! blockTypeImplementation ) {
return null;
}

return {
...blockTypeDefinition,
...blockTypeImplementation,
Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to me that the implementation can override attributes that are in the definition.

What about the icon? It's considered as implementation for now mainly because we allow svg elements which can't be defined server-side. An option would be to transform it to svg string to allow it to be moved to the definition of the block.

We could have icon supported both in addBlockType and implementBlockType. SVGs would only be supported in implementBlockType. The attribute specified in the implementation takes priority, as above.

Copy link
Member

Choose a reason for hiding this comment

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

It makes sense to me that the implementation can override attributes that are in the definition.

It's not quite as obvious to me. What's the use case?

attributes: mapValues( blockTypeDefinition.attributes, ( attribute, key ) => {
const implementationAttribute = get( blockTypeImplementation.attributes, [ key ], {} );
return {
...attribute,
...implementationAttribute,
};
} ),
};
},
( state ) => [
state.blockTypes,
state.implementations,
]
);

/**
* Returns all the available categories.
Expand Down
7 changes: 6 additions & 1 deletion core-blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
setDefaultBlockName,
setUnknownTypeHandlerName,
} from '@wordpress/blocks';
import { dispatch } from '@wordpress/data';

/**
* Internal dependencies
Expand Down Expand Up @@ -77,11 +78,15 @@ export const registerCoreBlocks = () => {
table,
textColumns,
verse,
video,
].forEach( ( { name, settings } ) => {
registerBlockType( name, settings );
} );

[ video ].forEach( ( { name, definition, implementation } ) => {
dispatch( 'core/blocks' ).addBlockTypes( { name, ...definition } );
Copy link
Member

Choose a reason for hiding this comment

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

I think this is not acceptable solution because it will break all plugins that depend on the following filter:

settings = applyFilters( 'blocks.registerBlockType', settings, name );

Copy link
Member

Choose a reason for hiding this comment

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

In addition this code bypass all validations we have in registerBlockType function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good points.

The filter

How to approach this? should we just leave the video block as is for now and refactor all the blocks at once once we create the alternative filters and move things to the server? I'm trying to avoid telling people to rewrite their code twice and only introduce deprecations once we have everything.

In addition this code bypass all validations we have in registerBlockType function.

I guess we should split validation between the two actions. I'll give it a shot

Copy link
Member

Choose a reason for hiding this comment

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

Two times yes :)

It was good for illustration how this can work, but we still need those wrapping API functions to make sure everything is valid.

dispatch( 'core/blocks' ).implementBlockTypes( { name, ...implementation } );
} );

setDefaultBlockName( paragraph.name );
setUnknownTypeHandlerName( freeform.name );
};
20 changes: 13 additions & 7 deletions core-blocks/video/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,27 +16,33 @@ import edit from './edit';

export const name = 'core/video';

export const settings = {
export const definition = {
title: __( 'Video' ),

description: __( 'Embed an video file and a simple video player.' ),

icon: 'format-video',

category: 'common',

attributes: {
id: {
type: 'number',
},
src: {
type: 'string',
},
caption: {
type: 'array',
},
},
};

export const implementation = {
icon: 'format-video',

attributes: {
src: {
source: 'attribute',
selector: 'video',
attribute: 'src',
},
caption: {
type: 'array',
source: 'children',
selector: 'figcaption',
},
Expand Down
18 changes: 17 additions & 1 deletion core-blocks/video/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,27 @@
/**
* External dependencies
*/
import { get, mapValues } from 'lodash';

/**
* Internal dependencies
*/
import { name, settings } from '../';
import { name, definition, implementation } from '../';
import { blockEditRender } from '../../test/helpers';

describe( 'core/video', () => {
test( 'block edit matches snapshot', () => {
const settings = {
...definition,
...implementation,
attributes: mapValues( definition.attributes, ( attribute, key ) => {
const implementationAttribute = get( implementation.attributes, [ key ], {} );
return {
...attribute,
...implementationAttribute,
};
} ),
};
const wrapper = blockEditRender( name, settings );

expect( wrapper ).toMatchSnapshot();
Expand Down