Incorporate getBlockLabel into useBlockDisplayInformation#29010
Incorporate getBlockLabel into useBlockDisplayInformation#29010david-szabo97 wants to merge 6 commits intotrunkfrom
Conversation
david-szabo97
added
[Package] Block editor
david-szabo97
requested review from
Addison-Stavlo,
Copons,
gziolo,
jeyip,
mattwiebe,
mtias,
ntsekouras and
vindl
|
Size Change: +2 B (0%) Total Size: 1.38 MB
ℹ️ View Unchanged
|
TestingBehavior
Browsers
Notes:
|
jeyip
left a comment
jeyip
left a comment
There was a problem hiding this comment.
Would it be possible to add a small e2e test to verify that the block inspector displays the template part name?
packages/block-editor/src/components/use-block-display-information/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/README.md
Outdated
Show resolved
Hide resolved
packages/block-editor/src/components/use-block-display-information/README.md
Outdated
Show resolved
Hide resolved
| const dynamicTitle = getBlockLabel( blockType, attributes ); | ||
| const variationInfo = getVariationInfo( variations, attributes ); | ||
|
|
||
| const title = |
There was a problem hiding this comment.
The use of the getBlockLabel to generate a "dynamic" title makes sense to me, but, with that being said, I don't have a lot of historical context with use-block-display-information.
Also, @david-szabo97, do you happen to know what the exact differences are between a block label and title? The label appears to be a canonical name, while the title is something customized by the user (definitely correct me if I'm wrong).
There was a problem hiding this comment.
The label appears to be a canonical name, while the title is something customized by the user (definitely correct me if I'm wrong).
Title is generally the hard-coded block name (e.g. 'Heading'), the label is the value returned by the __experimentalLabel property on a block's declaration. It's generally one of the attributes that represents what the user has 'labelled' a block, but it's not defined for every block.
edit: sorry, revised this comment a few times as my first interpretation was wrong.
There was a problem hiding this comment.
Oh I see -- I had it reversed in my head. This makes sense. Thanks @talldan!
jeyip
left a comment
jeyip
left a comment
There was a problem hiding this comment.
+1 from me when all comments are addressed
|
I want to confirm that the helper introduced in #18132 by @talldan is used as intended. As far as I can tell, Separately from this PR, should we promote this API to the stable name like |
|
Thanks for the review ping. From a user experience perspective, I'd be a bit nervous about removing one of the few places we display the actual title of a block (e.g. Heading, Link, Template Part).
I think it needs some work before becoming stable. I had a PR open to tackle some improvements - #19664 - but there was so much disagreement that it never really progressed. One of the good bits of feedback though was making |
jameskoster
left a comment
jameskoster
left a comment
There was a problem hiding this comment.
It works :)
From a user experience perspective, I'd be a bit nervous about removing one of the few places we display the actual title of a block (e.g. Heading, Link, Template Part).
I think this will be improved (for template parts at least) when we implement the changes suggested in #26599
david-szabo97
requested review from
ajitbohra,
nerrad and
ntwb
as code owners
Correct! Anything that references Added an e2e test and updated the comments. |
|
I don't have much to say about the code, but I wonder if this might make the UX worse. 🤔 As far as I can see, right now there are 2 core blocks making use of the Navigation Link can afford to use a dynamic title as its descriptions are... descriptive enough (e.g. "Add a link to a URL"). Template Part's description, on the other hand, kinda rely on the "Template Part" title to convey a complete meaning: "Edit the different global regions of your site, like the header, footer, sidebar, or create your own". Doesn't this read awkward?
Would it make sense to have Template Part's label return something like (To be fair, though, I wonder if me clinging to the block title is a power user/geek bias, while most people wouldn't care at all.) |
|
I'd rather leave the block inspector untouched here (reflecting the default block name). The idea for displaying "Header" there will be to create a block variation of the template part block set to the semantic taxonomy of |
|
I think that colorising template parts, and using the new icons will help (26599). It may also be helpful to revise the existing description, so that it explicitly mentions template parts. Maybe something like:
We should probably explore that in a separate issue though. |
I echo @mtias on this. I think this seems block variations territory. |
|
Basically, a template part can have any name. So I don't think variations is a good fit here. Like, we can't create a variation for each template part, that doesn't sound good to me 🤔 |
Block Card seems more
While it makes sense to show the I'm not sure exactly how we can handle this as variations but is worth exploring. Maybe these explorations here could help: #29095? What do you think? |
We can create one for the supported categories and everything else outside of that can be a generic template part? If you check out the parent issue in the context of the milestone it's building up on previous item related to block variations:
|
Yes, correct. Generic template parts should just read as "template part" in the inspector. Same with reusable blocks. We use the "label name" in other contexts like List View, Toolbar, Breadcrumb. |
|
Superseded by #29122 |
johnbillion
deleted the
update/incorporate-get-label-into-use-block-diplay-information
branch
9 participants
Related #27876
Description
We introduced a function for blocks that returns a dynamic label based on the block's attributes:
__experimentalLabel. (Example)The Block inspector didn't take this into account. To make sure this function is taken into account throughout the codebase, this PR incorporates it into the already existing and widely used
useBlockDisplayInformationhook. This also simplified the code inBlockTitle.How has this been tested?
Template PartblockScreenshots
(Block inspector says "Header" instead of "Template Part")
Types of changes
New feature (non-breaking change which adds functionality)
Checklist: