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: Add initial API for Block Patterns #18270

Merged
merged 7 commits into from Nov 8, 2019

Conversation

@gziolo
Copy link
Member

gziolo commented Nov 4, 2019

Description

Part 1 of #16283.

This is my proposal for the initial API:

/**
 * An object describing a pattern defined for the block type.
 *
 * @typedef {Object} WPBlockPattern
 *
 * @property {string}  name          The unique and machine-readable name.
 * @property {string}  label         A human-readable label.
 * @property {WPIcon}  [icon]        An icon helping to visualize the pattern.
 * @property {boolean} [isDefault]   Indicates whether the current pattern is the default one.
 *                                   Defaults to `false`.
 * @property {Object}  [attributes]  Values which override block attributes.
 * @property {Array[]} [innerBlocks] Initial configuration of nested blocks.
 */

innerBlocks needs more thoughts. I would prefer to defer this decision until the very first block is converted to use this new API. See more detailed explanation in #18270 (comment).

New APIs proposed in @wordpress/blocks

wp.data.*

/**
 * Registers a new block pattern for the given block.
 *
 * @param {string}         blockName Name of the block (example: “core/columns”).
 * @param {WPBlockPattern} pattern   Object describing a block pattern.
 */
function __experimentalRegisterBlockPattern( blockName, pattern );
/**
 * Unregisters a block pattern defined for the given block.
 *
 * @param {string} blockName   Name of the block (example: “core/columns”).
 * @param {string} patternName Name of the pattern defined for the block.
 */
function __experimentalUnregisterBlockPattern( blockName, patternName );

Actions

/**
 * Returns an action object used in signalling that new block patterns have been added.
 *
 * @param {string}                          blockName Block name.
 * @param {WPBlockPattern|WPBlockPattern[]} patterns  Block patterns.
 *
 * @return {Object} Action object.
 */
function __experimentalAddBlockPatterns( blockName, patterns );
/**
 * Returns an action object used in signalling that block patterns have been removed.
 *
 * @param {string}          blockName    Block name.
 * @param {string|string[]} patternNames Block pattern names.
 *
 * @return {Object} Action object.
 */
function __experimentalRemoveBlockPatterns( blockName, patternNames )

Selectors

/**
 * Returns block patterns by block name.
 *
 * @param {Object} state      Data state.
 * @param {string} blockName  Block type name.
 *
 * @return {?(WPBlockPattern[])} Block patterns.
 */
function __experimentalGetBlockPatterns( state, blockName );

How has this been tested?

npm run test

All added unit tests should pass.

*
* @return {Object} Updated state.
*/
export function blockPatterns( state = {}, action ) {

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 4, 2019

Author Member

This is nearly identical to blockStyles. In theory, we could extract duplicated code and abstract it away. However, I'm not convinced that this will make it easier to read in the long run. If you think this is a better way to go, I'm happy to refactor.

This comment has been minimized.

Copy link
@aduth

aduth Nov 7, 2019

Member

This is nearly identical to blockStyles. In theory, we could extract duplicated code and abstract it away. However, I'm not convinced that this will make it easier to read in the long run. If you think this is a better way to go, I'm happy to refactor.

Since we're inheriting the logic, I'm not overly concerned with addressing it here, but a point for both this and the blockStyles upon which it's based:

  • Do we really need to track a value for each registered block type? Can't this be something where we just manage the normalization to an empty array from the selector? Seems like it may make the implementation a little simpler, and avoid the (albeit minimal) memory usage involved with tracking a bunch of key/values for blocks without patterns.

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 8, 2019

Author Member

That's a good point, indeed. I guess this would force us to use memoization for the selector, but it seems like a good trade-off given that the implementation would get much simpler in the reducer. I'll open a follow-up shortly after I merge this one which is going to explore this idea for both reducers.

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 8, 2019

Author Member

Opened #18398 to keep track of it.

@gziolo gziolo requested a review from youknowriad Nov 4, 2019
@gziolo gziolo referenced this pull request Nov 4, 2019
3 of 9 tasks complete
@gziolo gziolo requested review from mtias and mcsf Nov 4, 2019
@gziolo gziolo force-pushed the add/patterns-api branch from 66f451f to 2cc172b Nov 5, 2019
* @param {string} blockName Name of the block (example: “core/columns”).
* @param {WPBlockPattern} pattern Object describing a block pattern.
*/
export const __experimentalRegisterBlockPattern = ( blockName, pattern ) => {

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 5, 2019

Author Member

@youknowriad - do we even need those APIs in the first place? I followed registerBlockStyle and unregisterBlockStyle but we could also use data API directly. What do you think?

This comment has been minimized.

Copy link
@aduth

aduth Nov 7, 2019

Member

@youknowriad - do we even need those APIs in the first place? I followed registerBlockStyle and unregisterBlockStyle but we could also use data API directly. What do you think?

In my opinion, I think we should have them:

  • It's slightly more convenient / obvious to use, than via the equivalent data actions
  • Consistency for consistency's sake

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 8, 2019

Author Member

I used this wp.blocks.* API in the examples presenting how you can tweak the Columns block with custom patterns so maybe it's indeed more convenient. The benefit is also that you don't have to worry whether the core/blocks store is initialized as it's embedded in the module. Wheres using the data module directly is prone to error in that regard.

@gziolo gziolo marked this pull request as ready for review Nov 5, 2019
@gziolo gziolo requested review from ajitbohra, ellatrix, nerrad and ntwb as code owners Nov 5, 2019
@gziolo gziolo referenced this pull request Nov 5, 2019
5 of 5 tasks complete
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 6, 2019

innerBlocks needs more thoughts. I would prefer to defer this decision until the very first block is converted to use this new API.

Based on #18283, is there any concern with this just being the block templates array syntax? I'm not really sure what's hinted at here as requiring further consideration.

@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Nov 7, 2019

innerBlocks needs more thoughts. I would prefer to defer this decision until the very first block is converted to use this new API.

Based on #18283, is there any concern with this just being the block templates array syntax? I'm not really sure what's hinted at here as requiring further consideration.

I don't have any concerns about this part. It's super solid, well-documented and extensively tested on production. I have been thinking about the name which is less of an issue but I noticed that we use template and innerBlocks to describe the same concepts. I'd love we use only one of them moving forward in all places. It probably would be a good idea to perform an audit and introduce an alias for backward compatibility, in effect start using only one of those names. The other thing I was contemplating is related config options you usually see with templates like the list of allowed blocks or the type of locking. I don't know if we should plan ahead to include them in the proposal as well.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 7, 2019

I don't have any concerns about this part. It's super solid, well-documented and extensively tested on production. I have been thinking about the name which is less of an issue but I noticed that we use template and innerBlocks to describe the same concepts. I'd love we use only one of them moving forward in all places. It probably would be a good idea to perform an audit and introduce an alias for backward compatibility, in effect start using only one of those names.

It's a good point. I'm not sure what the best option is here, since innerBlocks doesn't make much sense in the context of the post type template, and conversely template is also not very obvious in describing the (for lack of a better term) inner blocks of a block pattern.

Another option, if we standardize on "template" as the name for this format, is to include a prefix describing it for a pattern as applying to its inner blocks, i.e. innerBlocksTemplate.

@aduth
aduth approved these changes Nov 7, 2019
Copy link
Member

aduth left a comment

Looks pretty flawless to me! A few minor points, but nothing blocking.

* @param {string} blockName Name of the block (example: “core/columns”).
* @param {WPBlockPattern} pattern Object describing a block pattern.
*/
export const __experimentalRegisterBlockPattern = ( blockName, pattern ) => {

This comment has been minimized.

Copy link
@aduth

aduth Nov 7, 2019

Member

@youknowriad - do we even need those APIs in the first place? I followed registerBlockStyle and unregisterBlockStyle but we could also use data API directly. What do you think?

In my opinion, I think we should have them:

  • It's slightly more convenient / obvious to use, than via the equivalent data actions
  • Consistency for consistency's sake
*
* @return {Object} Updated state.
*/
export function blockPatterns( state = {}, action ) {

This comment has been minimized.

Copy link
@aduth

aduth Nov 7, 2019

Member

This is nearly identical to blockStyles. In theory, we could extract duplicated code and abstract it away. However, I'm not convinced that this will make it easier to read in the long run. If you think this is a better way to go, I'm happy to refactor.

Since we're inheriting the logic, I'm not overly concerned with addressing it here, but a point for both this and the blockStyles upon which it's based:

  • Do we really need to track a value for each registered block type? Can't this be something where we just manage the normalization to an empty array from the selector? Seems like it may make the implementation a little simpler, and avoid the (albeit minimal) memory usage involved with tracking a bunch of key/values for blocks without patterns.
packages/blocks/src/store/selectors.js Outdated Show resolved Hide resolved
Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
@gziolo

This comment has been minimized.

Copy link
Member Author

gziolo commented Nov 8, 2019

Another option, if we standardize on "template" as the name for this format, is to include a prefix describing it for a pattern as applying to its inner blocks, i.e. innerBlocksTemplate.

Yes, this one is longer but it probably fits better. @youknowriad what do you think? I know that you used innerBlocks in the context of example setting for blocks which I followed here:

example: {
innerBlocks: [
{
name: 'core/column',
innerBlocks: [

@gziolo gziolo merged commit a71241c into master Nov 8, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@gziolo gziolo deleted the add/patterns-api branch Nov 8, 2019
Copy link
Contributor

mcsf left a comment

Nice work.

return uniqBy( [
...get( blockType, [ 'patterns' ], [] ),
...get( state, [ blockType.name ], [] ),
], ( style ) => style.name );

This comment has been minimized.

Copy link
@mcsf

mcsf Nov 8, 2019

Contributor

I think this should read ( pattern ) => pattern.name :)

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 8, 2019

Author Member

Oooops 😂

This comment has been minimized.

Copy link
@gziolo

gziolo Nov 12, 2019

Author Member

Refactor planned in #18398 is going to fix it.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Nov 11, 2019

nice work here 👍

@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
* Blocks: Add initial API for Block Patterns

* Update packages/blocks/src/api/registration.js

* Convert all public APIs to experimental

* Expose block patterns registration APIs

* Fix typo in the name of selector

* Blocks: Update Patterns API integration with the store

* Update packages/blocks/src/store/selectors.js

Co-Authored-By: Andrew Duthie <andrew@andrewduthie.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.