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

Docs and API: clarify naming around block registration #842

Merged
merged 12 commits into from
Jun 1, 2017

Conversation

mtias
Copy link
Member

@mtias mtias commented May 19, 2017

There is some confusion around block instances as consumed in the editor and the registration API. We have two functions for getBlocks that return different things.

At the same time, we are keeping the block definition behind a getter like getBlockSettings where it is not immediately clear what the return value would be. (i.e. we return the save and edit render methods there.)

These are mainly relevant internally, but still important to be as clear as possible.


I propose we do one of the following:

  • /registration.js: getRegisteredBlock( name ) replaces getBlockSettings( slug ).
  • /registration.js: rename getBlocks to getRegisteredBlocks.

Or...

  • /registration.js: getBlockType( name ) replaces getBlockSettings( slug ).
  • /registration.js: rename getBlocks to getBlockTypes.

When retrieving settings and using them:

const blockType = getRegisteredBlock( name ) || getBlockType( name );
const EditBlock = blockType.edit;

@mtias mtias added [Status] In Progress Tracking issues with work in progress [Feature] Block API API that allows to express the block paradigm. [Type] Developer Documentation Documentation for developers Framework Issues related to broader framework topics, especially as it relates to javascript labels May 19, 2017
const blockSettings = getBlockSettings( blockType );

// Do we need this? What purpose does it have?
Copy link
Member

Choose a reason for hiding this comment

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

Let's create a separate issue for this. I think our current use-cases don't really need it as much as it seems they would.

@aduth
Copy link
Member

aduth commented May 19, 2017

I like registerBlock, getRegisteredBlock, and getRegisteredBlocks

More characters, but in my opinion best phrasing to indicate working with the "blueprint" of a block.

We'd probably want to be consistent with variable assignment too. const registeredBlock is a tad verbose but consistent with the function naming. blockType or blockSettings work well. block is a little non-descriptive for my liking.

@ehg
Copy link
Contributor

ehg commented May 22, 2017

More characters, but in my opinion best phrasing to indicate working with the "blueprint" of a block.

This is exactly how I've been thinking about the instance vs blueprint/prototype of a block. I prefer the explicit naming of a type vs. instance though, so would go for getBlockType. This feels a bit like React's element/component/component instance differences.

I really don't like blockSettings. It was confusing that these 'settings' contained functions, e.g. edit and save — which are verbs, but that's another story.

@aduth
Copy link
Member

aduth commented May 24, 2017

Yeah, "type" vocabulary is growing on me. I think I didn't like the distinction between registerBlock and getBlockTypes, but maybe if we were to name it registerBlockType ?

registerBlockType( 'core/text', {} );
const blockTypes = getBlockTypes();
const blockType = getBlockType( 'core/text' );

@ehg
Copy link
Contributor

ehg commented May 24, 2017 via email

@mtias mtias removed the [Status] In Progress Tracking issues with work in progress label May 30, 2017
blockSettings = getBlockSettings( blockType );
export function createBlockWithFallback( name, rawContent, attributes ) {
// Use type from block content, otherwise find unknown handler.
name = name || getUnknownTypeHandler();
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to standardize on name vs. slug ?

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 believe it to be clearer (slug usually not including slashes).

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 there's a few instances where we're already using slug.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I'm fine with cleaning up later.

blocks/README.md Outdated
@@ -146,7 +146,7 @@ Registers a new block-level control. Controls appear in a block's toolbar when i

Inline controls for [`Editable`](#editable) elements are identical for every block and cannot be modified.

### `wp.blocks.getBlockSettings( slug: string )`
### `wp.blocks.getBlockType( slug: string )`

Returns settings associated with a registered block.
Copy link
Contributor

Choose a reason for hiding this comment

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

settings? :)

Copy link
Member

Choose a reason for hiding this comment

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

settings? :)

I'm conflicted on this. What do we name the second argument to registerBlockType? Currently it's still settings. The "type" terminology doesn't apply as well. Maybe something more akin to "type definition", or configuration.

@mtias mtias added the [Priority] High Used to indicate top priority items that need quick attention label May 31, 2017
* @param {Object} attributes Block attributes
* @return {Object} Block object
*/
export function createBlock( blockType, attributes = {} ) {
const blockSettings = getBlockSettings( blockType );
export function createBlock( blockName, attributes = {} ) {
Copy link
Contributor

@BE-Webdesign BE-Webdesign May 31, 2017

Choose a reason for hiding this comment

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

I think blockName should remain blockType. Where you can interpret the value to be the ID of the blockType. (ignore this comment as I didn't read further down to realize that would be a catastrophe 😱 )

Copy link
Contributor

@BE-Webdesign BE-Webdesign May 31, 2017

Choose a reason for hiding this comment

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

Actually maybe typeOfBlock then you can keep the new changes. That is what the string value is.

const blockSettings = getBlockSettings( blockType );
export function createBlock( blockName, attributes = {} ) {
// Get the type definition associated with a registered block.
const blockType = getBlockType( blockName );
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 purpose of the PR is precisely to use blockType as the bespoke "prototype" or type description of the block, not the name, not the block instance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, you can disregard most of my comments as this is fine. It was somewhat confusing to me at first.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

I tried testing everything. LGTM 👍

@mtias mtias merged commit bdabb3f into master Jun 1, 2017
@mtias
Copy link
Member Author

mtias commented Jun 1, 2017

Thanks!

@mtias mtias deleted the update/docs-and-api branch June 1, 2017 11:41
@aduth
Copy link
Member

aduth commented Jun 1, 2017

Nice one 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. Framework Issues related to broader framework topics, especially as it relates to javascript [Priority] High Used to indicate top priority items that need quick attention [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants