Try: Add metadata attribute to blocks allowing section naming and future semantic meta information #40393
Conversation
jorgefilipecosta
added
[Block] Group
|
Size Change: +145 B (0%) Total Size: 1.23 MB
ℹ️ View Unchanged
|
jorgefilipecosta
force-pushed
the
add/section-concept
branch
from
2567244
to
c4f27a0
Compare
|
I added a unit test to this PR. |
I wonder if this should be a unique attribute |
I had the same doubt I can go for the |
jorgefilipecosta
force-pushed
the
add/section-concept
branch
from
c4f27a0
to
698c1c2
Compare
|
Hi @youknowriad,
I'm worried about another important topic the section title needs to be translatable. If the pattern is defined in PHP we can use the normal PHP i18n mechanisms but for the patterns on the pattern, the directory is this may be an issue. I'm not sure how are the sections in the pattern directory going to be translatable. Should we have a special mechanism to extract strings from these attributes in order to make them translatable? |
|
A thought cross my mind while reading this comment and specifically, the fact that we could also support "naming" any block and not just section. Maybe what we need is not a "section" attribute but a "metadata" attribute where we could set things like "name", "description"... for any blocks potentially (surface only for sections at first) and the same metadata attribute could contain an "isSection" flag. It's still unclear to me if the "isSection" flag is actually needed or if it's just a temporary thing while we implement other meaningful metadata like
I think it's the same issue we have for content inside patterns as well. I don't really think we need a special i18n mechanism. But I may be missing something? |
jorgefilipecosta
force-pushed
the
add/section-concept
branch
from
698c1c2
to
30ca4ac
Compare
|
Hi @youknowriad, I followed your suggestion, now we have a metadata attribute with "name" and a isSection attribute. For now, everything is restricted to sections and blocks that declare support for sections. Regarding i18n yes we can follow what happens for pattern content. But the issue of internationalization of patterns even for content is not solved. e.g: on the pattern directory all the patterns are in English and I can not get patterns in another language. But I guess that's a separate discussion. This PR seems ready. |
| * @return {Object} Filtered block settings. | ||
| */ | ||
| export function addAttribute( settings ) { | ||
| if ( hasBlockSupport( settings, '__experimentalSection', false ) ) { |
There was a problem hiding this comment.
Feels to me that we should have the "metadata" attribute for all blocks, not just sections but I guess that's a fine start.
| export function addAttribute( settings ) { | ||
| if ( hasBlockSupport( settings, '__experimentalSection', false ) ) { | ||
| // Allow blocks to specify their own metadata attribute definition with default value if needed. | ||
| if ( ! has( settings.attributes, [ 'metadata' ] ) ) { |
There was a problem hiding this comment.
It would be cool to do a dev note to let people know that this might become a "reserved" attribute. We should also document that "metadata", "style", "settings" and "lock" are all reserved attributes.
Writing this makes me thing that "lock" could actually be a child of "metadata" instead of being its own attribute.
There was a problem hiding this comment.
Are we still on time to make lock a child of metadata? I think it is also fine to have lock as a standalone attribute.
There was a problem hiding this comment.
Probably too late unless we migrate later (so nothing to do now)
| const label = reusableBlockTitle || blockLabel; | ||
| const sectionTitle = | ||
| hasBlockSupport( name, '__experimentalSection', false ) && | ||
| attributes.metadata?.isSection && |
There was a problem hiding this comment.
Do we really need to check isSection here :) Why not just allow any blocks to have a name?
| firstBlockName, | ||
| '__experimentalSection', | ||
| false | ||
| ) && !! getBlockAttributes( clientId ).metadata?.isSection, |
There was a problem hiding this comment.
Should this check be more something like hasName: getBlockAttributes( clientId ).metadata?.name.
There was a problem hiding this comment.
This PR looks very close to me. I'm missing two things:
- A bit of documentation maybe
- Some thoughts from more folks about whether to introduce explicitly the "isSection" flag here or if it's more about just adding a "name" to any block and a "metadata" attribute (that can absorb more things later)
I'm thinking the latter is better than introducing yet another potentially confusing concept. We'd be able to achieve the same thing but instead of thinking: these are section blocks, we'd be more thinking these are blocks with more capabilities (name, semantics...)
|
The latter makes more sense to me as well. "Section" just feels like an abstract description for any root level block. In that sense, everything is a section depending on context. |
@youknowriad and @jorgefilipecosta , I'm missing a full context here. This is what I understood so far. There is this concept to mark some blocks to use the new UI feature of sections. You can use the new flag Does it mean that for now it mostly allows setting a different name for the block title in the List View, the block toolbar, and probably a few other places? It reminds me a bit title used with Reusable Blocks and Template Parts. For example for the custom handling in |
|
Yes, that's a good summary.
metadata.name is exactly like that, a way to give a "name" to any block (regardless if it's reusable or not). There are probably some relationships to be made with both "template parts" and "reusable blocks" names. For instance when transforming from a regular block to reusable, pre-populate the reusable block's name...
Yes, I'm thinking that we should remove the isSection for now personally. |
jorgefilipecosta
force-pushed
the
add/section-concept
branch
from
30ca4ac
to
d0b01f2
Compare
jorgefilipecosta
requested review from
ryanwelcher and
juanmaguitar
as code owners
|
Thank you @youknowriad, @jameskoster, and @gziolo for the insights. I applied all the feedback and added some documentation. |
| if ( ! has( settings.attributes, [ 'metadata' ] ) ) { | ||
| settings.attributes = { | ||
| ...settings.attributes, | ||
| metadata: { |
There was a problem hiding this comment.
What would be the difference with the new settings attribute added in #40547 and the meta attribute proposed here?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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:
lockallowedBlocks, exampletemplateLock, example
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.
There was a problem hiding this comment.
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',
},There was a problem hiding this comment.
@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)
There was a problem hiding this comment.
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..
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
jorgefilipecosta
force-pushed
the
add/section-concept
branch
2 times, most recently
from
eab5108
to
fd5ff69
Compare
|
Following a suggestion by @mcsf this PR is now using __experimentalMetadata instead of __experimentalSection. |
jorgefilipecosta
changed the title
Squashed commits: [c1bd86cd5d] Use section object [7667d1715e] Add unit test [e7d647eb76] Add: Section concept
jorgefilipecosta
force-pushed
the
add/section-concept
branch
from
fd5ff69
to
871b7ed
Compare
| case 'name-with-section': | ||
| return { title: 'Block With Section Support' }; |
There was a problem hiding this comment.
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.
|
I've been heading in a similar direction over at #42605. I'd like to reconcile with what's happening here. |
|
@jorgefilipecosta and I spoke away from Github and we agreed I would borrow items from this PR for #42605 with a view that this one may end up being closed. |
|
Closing this PR as #42605 is continuing this work. |
Fixes: #40317
This PR adds an experimental support mechanism
"__experimentalSection"__experimentalMetadatathat any block can use to mark that a block supports being a section.It enables
"__experimentalSection"__experimentalMetadataon the group block.When a block supports being a section two attributes are automatically added, isSection defaulting to false and sectionName. When isSection attribute is true it means that the given block instance is a section. sectionName contains a description of the section e.g."404 Section" to show on the UI and can also be used to show similar patterns (e.g: patterns containing sections with the same name).
The isSection attribute is a marker to know the sections we have. We should update #40314 and or #40376 to use this marker.
By default, we don't have any UI to control these attributes. But a block can have a UI e.g: A text field or select control to select the name of the section. A block may also register variations that are sections etc.
We are also updating the UI to show the section name has the block title as we do for reusable blocks and template parts.
Testing Instructions
I added the following block to the block editor:
I verified the UI showed the section name as the screenshots above show.