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

Try: Add metadata attribute to blocks allowing section naming and future semantic meta information #40393

Closed
wants to merge 1 commit into from
Closed

Try: Add metadata attribute to blocks allowing section naming and future semantic meta information #40393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
14 changes: 14 additions & 0 deletions docs/reference-guides/block-api/block-supports.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,3 +664,17 @@ attributes: {
}
}
```

### __experimentalMetadata

- Type: `boolean`
- Default value: `false`

The block gets the `metadata` attribute added to the block definition when `__experimentalMetadata` is true. The `metadata` attribute has the type `object` without a default value provided. Blocks can manually add this attribute and set their defaults.
The metadata attribute contains information related to the block.

For now, the following metadata properties are available:
- `name` - Contains a descriptive name of the block shown on the block switcher and blocklist view. E.g., A group block may be named "404 Section".

This functionality is experimental, and the properties could be added or removed.
In the future, `__experimentalMetadata` support may not exist, and the `metadata` attribute may be added unconditionally to all the blocks.
14 changes: 9 additions & 5 deletions packages/block-editor/src/components/block-switcher/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,18 +43,19 @@ export const BlockSwitcherDropdownMenu = ( { clientIds, blocks } ) => {
icon,
blockTitle,
patterns,
hasBlockName,
} = useSelect(
( select ) => {
const {
getBlockRootClientId,
getBlockTransformItems,
__experimentalGetPatternTransformItems,
getBlockAttributes,
} = select( blockEditorStore );
const { getBlockStyles, getBlockType } = select( blocksStore );
const { canRemoveBlocks } = select( blockEditorStore );
const rootClientId = getBlockRootClientId(
castArray( clientIds )[ 0 ]
);
const clientId = castArray( clientIds )[ 0 ];
const rootClientId = getBlockRootClientId( clientId );
const [ { name: firstBlockName } ] = blocks;
const _isSingleBlockSelected = blocks.length === 1;
const styles =
Expand Down Expand Up @@ -84,6 +85,7 @@ export const BlockSwitcherDropdownMenu = ( { clientIds, blocks } ) => {
blocks,
rootClientId
),
hasBlockName: !! getBlockAttributes( clientId ).metadata?.name,
};
},
[ clientIds, blocks, blockInformation?.icon ]
Expand Down Expand Up @@ -111,7 +113,7 @@ export const BlockSwitcherDropdownMenu = ( { clientIds, blocks } ) => {
icon={
<>
<BlockIcon icon={ icon } showColors />
{ ( isReusable || isTemplate ) && (
{ ( hasBlockName || isReusable || isTemplate ) && (
<span className="block-editor-block-switcher__toggle-text">
<BlockTitle
clientId={ clientIds }
Expand Down Expand Up @@ -168,7 +170,9 @@ export const BlockSwitcherDropdownMenu = ( { clientIds, blocks } ) => {
className="block-editor-block-switcher__toggle"
showColors
/>
{ ( isReusable || isTemplate ) && (
{ ( isReusable ||
isTemplate ||
hasBlockName ) && (
<span className="block-editor-block-switcher__toggle-text">
<BlockTitle
clientId={ clientIds }
Expand Down
22 changes: 22 additions & 0 deletions packages/block-editor/src/components/block-title/test/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ jest.mock( '@wordpress/blocks', () => {

case 'name-with-long-label':
return { title: 'Block With Long Label' };
case 'name-with-section':
return { title: 'Block With Section Support' };
Comment on lines +34 to +35
Copy link
Contributor

Choose a reason for hiding this comment

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

With the change of intent in this PR (moving away from referring to "section"), should this also be reflected in the phrasing/naming in tests? I think leaving as is could be confusing for those reading tests to understand expected behaviour.

}
},
__experimentalGetBlockLabel( { title } ) {
Expand All @@ -48,6 +50,11 @@ jest.mock( '@wordpress/blocks', () => {
return title;
}
},
hasBlockSupport( name, support, defaultSupport = false ) {
if ( support === '__experimentalMetadata' ) {
return name === 'name-with-section' ? true : defaultSupport;
}
},
};
} );

Expand Down Expand Up @@ -178,4 +185,19 @@ describe( 'BlockTitle', () => {
'This is a longer label than typical for blocks to have.'
);
} );

it( 'should return section title if the block supports it', () => {
useSelect.mockImplementation( () => ( {
name: 'name-with-section',
attributes: {
metadata: { name: 'My custom section' },
},
} ) );

const wrapper = shallow(
<BlockTitle clientId="id-name-with-long-label" />
);

expect( wrapper.text() ).toBe( 'My custom section' );
} );
} );
4 changes: 3 additions & 1 deletion packages/block-editor/src/components/block-title/use-block-display-title.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ export default function useBlockDisplayTitle( clientId, maximumLength ) {
? getBlockLabel( blockType, attributes )
: null;

const label = reusableBlockTitle || blockLabel;
const blockName = attributes?.metadata?.name;

const label = blockName || reusableBlockTitle || blockLabel;
// Label will fallback to the title if no label is defined for the current
// label context. If the label is defined we prioritize it over a
// possible block variation title match.
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/hooks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import './compat';
import './align';
import './lock';
import './metadata';
import './anchor';
import './custom-class-name';
import './generated-class-name';
Expand Down
40 changes: 40 additions & 0 deletions packages/block-editor/src/hooks/metadata.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
/**
* External dependencies
*/
import { has } from 'lodash';

/**
* WordPress dependencies
*/
import { addFilter } from '@wordpress/hooks';
import { hasBlockSupport } from '@wordpress/blocks';

/**
* Filters registered block settings, extending attributes to include `metadata` in blocks declaring section support.
*
* @param {Object} blockTypeSettings Original block settings.
*
* @return {Object} Filtered block settings.
*/
export function addAttribute( blockTypeSettings ) {
if (
hasBlockSupport( blockTypeSettings, '__experimentalMetadata', false )
) {
// Allow blocks to specify their own metadata attribute definition with default value if needed.
if ( ! has( blockTypeSettings.attributes, [ 'metadata' ] ) ) {
blockTypeSettings.attributes = {
...blockTypeSettings.attributes,
metadata: {
Copy link
Member

Choose a reason for hiding this comment

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

What would be the difference with the new settings attribute added in #40547 and the meta attribute proposed here?

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 settings attribute is a global styles settings object like the presets of a block, and other settings you can define using theme.json. It is equivalent to the styles object. The metadata is for information specific to a block instance that is not part of global styles/theme.json like the name of a specific section.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is though a good question (whether these could merge or not). I'm not sure I have any preference right now. They do seem to have different purposes.

Copy link
Member

Choose a reason for hiding this comment

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

I'm really worried that we overuse settings and metadata which already exist in Block API but play a different role.

We use the block metadata phrase when talking about everything that block.json can represent:
https://github.com/WordPress/gutenberg/blob/trunk/docs/reference-guides/block-api/block-metadata.md

For settings the best way to show the challenge is by showing the implementation:

gutenberg/packages/block-editor/src/hooks/settings.js

Lines 16 to 23 in fa456ec

if ( ! settings?.attributes?.settings ) {
settings.attributes = {
...settings.attributes,
settings: {
type: 'object',
},
};
}

In effect, settings is an attribute in the block settings' object during registration.

There are also existing blocks that contain similar types of attributes that don't represent the content or visual aspects of the block and are included as regular attributes:

I agree that it doesn't have too much difference from the implementation perspective and it's perfectly fine to proceed as is with settings and metdata attributes. However, it might be challenging to document all the subtle differences knowing about all the ambiguity introduced.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is though a good question (whether these could merge or not). I'm not sure I have any preference right now. They do seem to have different purposes.

Just wanted to say that this has been on my mind since yesterday as well. And not only is metadata an overloaded term (block type metadata, post meta, Redux action meta, etc.), it's also a potential magnet for any random data. @jorgefilipecosta and @youknowriad: besides name, what do we contemplate storing in it?

In effect, settings is an attribute in the block settings' object during registration.

I agree, @gziolo, that settings is also a bit overloaded, but it's also a first-class name in theme.json, and attributes.settings is symmetric with attributes.style, so IMO we could keep it. Maybe we could make the code clearer with more specific naming — for instance:

-       if ( ! settings?.attributes?.settings ) {
-               settings.attributes = {
-                       ...settings.attributes,
+       if ( ! blockTypeSettings?.attributes?.settings ) {
+               blockTypeSettings.attributes = {
+                       ...blockTypeSettings.attributes,
                        settings: {
                                type: 'object',
                        },

Copy link
Contributor

Choose a reason for hiding this comment

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

@jorgefilipecosta and @youknowriad: besides name, what do we contemplate storing in it?

The two certain things is "name" and somekind of "tag" or "area" or "keyword" or something like that to add "semantics" to the given section/block. (better surface related patterns...). I think "locking" could be moved in that same object.

Potentially other things can come later. This comment is relevant here #34352 (comment)

Copy link
Contributor

@ntsekouras ntsekouras May 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It's true that having all the existing block settings and the new settings and metadata can be confusing about what goes where. IMO the new settings attribute is targeted for the theme.json mechanism so we could rename that one to something more indicative about its purpose/contents?

Other than that the metadata seems just fine to me and we can always look to migrate some other props there in the future properly..

Copy link
Member Author

Choose a reason for hiding this comment

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

settings attribute is targeted for the theme.json mechanism so we could rename that one to something more indicative about its purpose/contents?

Theme.json has styles and settings it seems natural that blocks also have styles and settings attributes. We don't have any other attribute for settings.

Copy link
Member Author

Choose a reason for hiding this comment

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

We should follow @mcsf and use blockTypeSettings as a name to make things clearer. This PR already does that, I will update the settings hook.

type: 'object',
},
};
}
}
return blockTypeSettings;
}

addFilter(
'blocks.registerBlockType',
'core/metadata/addAttribute',
addAttribute
);
1 change: 1 addition & 0 deletions packages/block-library/src/group/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
"align": [ "wide", "full" ],
"anchor": true,
"html": false,
"__experimentalMetadata": true,
"color": {
"gradients": true,
"link": true,
Expand Down