Block settings dropdown: use block display title in remove label#39110
Conversation
|
Size Change: +33 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
andrewserong
left a comment
There was a problem hiding this comment.
Nice one @ramonjd — good idea abstracting the logic from the BlockTitle component and moving it to a hook with custom truncation length 👍.
This is testing nicely for me in the post and site editors, the List View display of the block title still looks good, and now the text for removing a Row block looks correct, along with removing a Reusable block:
| Removing a Row block | Removing a Reusable block |
|---|---|
 |
 |
I just left a tiny comment about where we use an example of the BlockTitle component, but this LGTM! Might be worth getting another pair of eyes on it too, just in case 🙂
It looks lit was updated to 35 chars. Anyway, I'm slightly concerned about using any default value for truncating the computed block's title. It will impact every usage of the |
|
Thanks for checking it out @gziolo ! 🙇
I left the default of 35 alone. It was bumped from 15 more than a year ago. The
The The goal was to avoid huge titles in the drop down and therefore very wide dropdown panels.
Agree that truncation is probably not appropriate in all cases. For the Block List Tree and, arguably, the drop down menus, it might be desirable to defend against loquacious labels. I'm happy to create another PR to audit the usage of It might be worthwhile adding an
As part of any follow up I can definitely update the documentation of the Block API to recommended shorter titles in general for blocks. Sounds like a very reasonable thing to communicate to folks. |
| * @param {string} clientId Client ID of block. | ||
| * @param {number} maximumLength The maximum length that the block title string may be before truncated. | ||
| * @return {?string} Block title. | ||
| */ | ||
| export default function useBlockDisplayTitle( clientId, maximumLength ) { | ||
| maximumLength = Number.isInteger( maximumLength ) | ||
| ? maximumLength | ||
| : MAXIMUM_TITLE_LENGTH; |
There was a problem hiding this comment.
Could maximumLength be treated as an optional param with a default value?
| * @param {string} clientId Client ID of block. | |
| * @param {number} maximumLength The maximum length that the block title string may be before truncated. | |
| * @return {?string} Block title. | |
| */ | |
| export default function useBlockDisplayTitle( clientId, maximumLength ) { | |
| maximumLength = Number.isInteger( maximumLength ) | |
| ? maximumLength | |
| : MAXIMUM_TITLE_LENGTH; | |
| * @param {string} clientId Client ID of block. | |
| * @param {number|undefined} maximumLength The maximum length that the block title string may be before truncated. | |
| * @return {?string} Block title. | |
| */ | |
| export default function useBlockDisplayTitle( clientId, maximumLength = MAXIMUM_TITLE_LENGTH ) { |
There was a problem hiding this comment.
Now that I've said that I've noticed that there doesn't seem to be a way to always get the full block title without any truncation. The only option is to use a really big number for the max length.
Because a new API is being introduced with this hook there's an opportunity to step away from the way BlockTitle did things.
The truncation in BlockTitle could still be left working as it was for back compat by passing in a max length param to the hook, but maybe when undefined is passed to the hook it should return the full block title (and not have an implicit default at all).
There was a problem hiding this comment.
Right, that was my initial thinking that isn't possible to show the full title with the revised implementation. There is some legacy handling here that truncates long titles for reusable blocks but it wasn't applied to other blocks. So maybe we could keep it as the default, and truncate everything when an explicit value is passed.
There was a problem hiding this comment.
Thanks for your help @talldan and @gziolo
maybe when undefined is passed to the hook it should return the full block title (and not have an implicit default at all
So maybe we could keep it as the default, and truncate everything when an explicit value is passed.
I agree that it makes a lot of sense if truncation is optional.
I've taken this route in f625f28 since it allows for greater flexibility. 👍
For all existing usages I'm passing 35 (the current default) for now to maintain the status quo and to keep this PR manageable.
If we want to revisit individual lengths, I'm happy to do it in a follow up PR. How does that sound?
…the drop down label, similar to the way the block list view does it. Add a maximum truncate value Updated tests
Pass `35` as maximumLength to maintain backwards compatibility for current usages Updated tests
a4f8483 to
f625f28
Compare
|
Updated in f625f28 to remove truncation by default. I've preserved the current, default truncation value of
|
| if ( label && label !== blockType.title ) { | ||
| return maximumLength && maximumLength > 0 | ||
| ? truncate( label, { length: maximumLength } ) | ||
| : label; | ||
| } |
There was a problem hiding this comment.
It's minor, but it might be worth mentioning in the docs for the maximumLength param that truncation only applies to the label (and not the title).
I'm not sure the best way to explain it, maybe mentioning that truncation only applies when the block displays something other than its title (e.g. a variation name or a custom label).
There was a problem hiding this comment.
Good point. Definitely worth communicating that. I'll throw up a small PR.
There was a problem hiding this comment.
What if the block title has 100 characters? 😄
Maybe we should also truncate it for consistency when someone explicitly passes this parameter.
There was a problem hiding this comment.
What if the block title has 100 characters? 😄
💥
Haha. Yeah, the expectation seems to have been that block title strings should be protected from truncation or were never going to be long enough to be truncated.
Maybe we should also truncate it for consistency when someone explicitly passes this parameter.
A cursory grep through the block.json files (there might be a few e2e fixtures in there) shows that there's not much over 28 chars in length, so we could probably get away with it.
Block title string lengths
Read More: 9
Site Logo: 9
Post Author: 11
Video: 5
Post Comments Link: 18
Comment Author Avatar: 21
Custom Link: 11
Comments Pagination: 19
Tag Cloud: 9
Calendar: 8
Buttons: 7
Post Date: 9
Comments Query Loop: 19
Embed: 5
Post Excerpt: 12
File: 4
Paragraph: 9
Quote: 5
Group: 5
Site Tagline: 12
Shortcode: 9
Comment Edit Link: 17
Separator: 9
Gallery: 7
Heading: 7
Comment Template: 16
Comment Content: 15
Term Description: 16
RSS: 3
Post Author Name: 16
Next Page: 9
Page Numbers: 12
Code: 4
Navigation: 10
Verse: 5
Post Comments Count: 19
Pullquote: 9
Post Author Biography: 21
Query Title: 11
Custom HTML: 11
Column: 6
Template Part: 13
Archives: 8
Social Icons: 12
Unsupported: 11
Spacer: 6
Social Icon: 11
Post Navigation Link: 20
Image: 5
More: 4
Table of Contents: 17
Search: 6
Page Break: 10
Text Columns (deprecated): 25
Comment Date: 12
Audio: 5
Post Comments Form: 18
Next Page: 9
Button: 6
Login/out: 9
Table: 5
Post Template: 13
List: 4
Preformatted: 12
Pagination: 10
Classic: 7
Post Terms: 10
Previous Page: 13
Navigation Area: 15
Home Link: 9
Post Featured Image: 19
Page List: 9
Previous Page: 13
Comment Author Name: 19
Columns: 7
Latest Comments: 15
Latest Posts: 12
Pattern: 7
Media & Text: 12
Post Content: 12
Post Title: 10
Query Loop: 10
Site Title: 10
Comment Reply Link: 18
Submenu: 7
Page Numbers: 12
Categories: 10
Post Comments: 13
Reusable block: 14
Post Comment (deprecated): 25
Cover: 5
Iframed Block: 13
Iframed Masonry Block: 21
Iframed Multiple Stylesheets: 28
Iframed Inline Styles: 21
Legacy Widget: 13
Notice: 6
Read More: 9
Site Logo: 9
Post Author: 11
Video: 5
Post Comments Link: 18
Comment Author Avatar: 21
Custom Link: 11
Comments Pagination: 19
Tag Cloud: 9
Calendar: 8
Buttons: 7
Post Date: 9
Comments Query Loop: 19
Embed: 5
Post Excerpt: 12
File: 4
Paragraph: 9
Quote: 5
Group: 5
Site Tagline: 12
Shortcode: 9
Comment Edit Link: 17
Separator: 9
Gallery: 7
Heading: 7
Comment Template: 16
Comment Content: 15
Term Description: 16
RSS: 3
Post Author Name: 16
Next Page: 9
Page Numbers: 12
Code: 4
Navigation: 10
Verse: 5
Post Comments Count: 19
Pullquote: 9
Post Author Biography: 21
Query Title: 11
Custom HTML: 11
Column: 6
Template Part: 13
Archives: 8
Social Icons: 12
Unsupported: 11
Spacer: 6
Social Icon: 11
Post Navigation Link: 20
Image: 5
More: 4
Table of Contents: 17
Search: 6
Page Break: 10
Text Columns (deprecated): 25
Comment Date: 12
Audio: 5
Post Comments Form: 18
Next Page: 9
Button: 6
Login/out: 9
Table: 5
Post Template: 13
List: 4
Preformatted: 12
Pagination: 10
Classic: 7
Post Terms: 10
Previous Page: 13
Navigation Area: 15
Home Link: 9
Post Featured Image: 19
Page List: 9
Previous Page: 13
Comment Author Name: 19
Columns: 7
Latest Comments: 15
Latest Posts: 12
Pattern: 7
Media & Text: 12
Post Content: 12
Post Title: 10
Query Loop: 10
Site Title: 10
Comment Reply Link: 18
Submenu: 7
Page Numbers: 12
Categories: 10
Post Comments: 13
Reusable block: 14
Post Comment (deprecated): 25
Cover: 5
Iframed Block: 13
Iframed Masonry Block: 21
Iframed Multiple Stylesheets: 28
Iframed Inline Styles: 21
Legacy Widget: 13
Read More: 9
Post Comment Author: 19
Site Logo: 9
Post Author: 11
Post Comment Date: 17
Video: 5
Post Comments Link: 18
Comment Author Avatar: 21
Comment Author Avatar: 21
Custom Link: 11
Comments Pagination: 19
Tag Cloud: 9
Calendar: 8
Buttons: 7
Post Date: 9
Comments Query Loop: 19
Embed: 5
Post Comment Content: 20
Post Excerpt: 12
File: 4
Paragraph: 9
Quote: 5
Group: 5
Site Tagline: 12
Shortcode: 9
Post Comment Reply Link: 23
Comment Edit Link: 17
Separator: 9
Gallery: 7
Heading: 7
Comment Template: 16
Comment Content: 15
Term Description: 16
RSS: 3
Post Author Name: 16
Next Page: 9
Page Numbers: 12
Code: 4
Navigation: 10
Verse: 5
Post Comments Count: 19
Pullquote: 9
Post Author Biography: 21
Query Title: 11
Custom HTML: 11
Column: 6
Template Part: 13
Archives: 8
Social Icons: 12
Unsupported: 11
Spacer: 6
Social Icon: 11
Post Navigation Link: 20
Image: 5
More: 4
Table of Contents: 17
Search: 6
Page Break: 10
Text Columns (deprecated): 25
Comment Date: 12
Audio: 5
Post Comments Form: 18
Next Page: 9
Button: 6
Login/out: 9
Table: 5
Post Template: 13
List: 4
Preformatted: 12
Pagination: 10
Classic: 7
Post Terms: 10
Previous Page: 13
Navigation Area: 15
Home Link: 9
Post Featured Image: 19
Page List: 9
Previous Page: 13
Comment Author Name: 19
Post Empty: 10
Post Comment Edit Link: 22
Columns: 7
Latest Comments: 15
Latest Posts: 12
Pattern: 7
Media & Text: 12
Post Content: 12
Post Title: 10
Query Loop: 10
Site Title: 10
Comment Reply Link: 18
Submenu: 7
Page Numbers: 12
Categories: 10
Post Comments: 13
Reusable block: 14
Post Comment (deprecated): 25
Cover: 5
Legacy Widget: 13
Notice: 6
Read More: 9
Site Logo: 9
Post Author: 11
Video: 5
Post Comments Link: 18
Comment Author Avatar: 21
Custom Link: 11
Comments Pagination: 19
Tag Cloud: 9
Calendar: 8
Buttons: 7
Post Date: 9
Comments Query Loop: 19
Embed: 5
Post Excerpt: 12
File: 4
Paragraph: 9
Quote: 5
Group: 5
Site Tagline: 12
Shortcode: 9
Comment Edit Link: 17
Separator: 9
Gallery: 7
Heading: 7
Comment Template: 16
Comment Content: 15
Term Description: 16
RSS: 3
Post Author Name: 16
Next Page: 9
Page Numbers: 12
Code: 4
Navigation: 10
Verse: 5
Post Comments Count: 19
Pullquote: 9
Post Author Biography: 21
Query Title: 11
Custom HTML: 11
Column: 6
Template Part: 13
Archives: 8
Social Icons: 12
Unsupported: 11
Spacer: 6
Social Icon: 11
Post Navigation Link: 20
Image: 5
More: 4
Table of Contents: 17
Search: 6
Page Break: 10
Text Columns (deprecated): 25
Comment Date: 12
Audio: 5
Post Comments Form: 18
Next Page: 9
Button: 6
Login/out: 9
Table: 5
Post Template: 13
List: 4
Preformatted: 12
Pagination: 10
Classic: 7
Post Terms: 10
Previous Page: 13
Navigation Area: 15
Home Link: 9
Post Featured Image: 19
Page List: 9
Previous Page: 13
Comment Author Name: 19
Columns: 7
Latest Comments: 15
Latest Posts: 12
Pattern: 7
Media & Text: 12
Post Content: 12
Post Title: 10
Query Loop: 10
Site Title: 10
Comment Reply Link: 18
Submenu: 7
Page Numbers: 12
Categories: 10
Post Comments: 13
Reusable block: 14
Post Comment (deprecated): 25
Cover: 5There was a problem hiding this comment.
If the truncation is applied to both the block title and the other labels, that's probably the point at which it doesn't need to be part of the hook.
The same code would just be:
const title = useBlockDisplayTitle( clientId );
const truncatedTitle = truncate( title, maxLength );There was a problem hiding this comment.
A cursory grep through the block.json files (there might be a few e2e fixtures in there) shows that there's not much over 28 chars in length, so we could probably get away with it.
Thank you for checking that in WordPress core. It looks like we can postpone the decision 👍🏻
It's not that I want to push for truncating block titles, but I had this thought that I wanted to share here. When you translate titles they might be longer. As far as I remember the German language has this tendency to be much longer than what you see in English.
There was a problem hiding this comment.
As far as I remember the German language has this tendency to be much longer than what you see in English.
That is an excellent point.
We might one day have a Rindfleischetikettierungsüberwachungsaufgabenübertragungsgesetz Block and be in real trouble. 😄
If the truncation is applied to both the block title and the other labels, that's probably the point at which it doesn't need to be part of the hook.
I think it's probably worth doing to make things work more logically for our future selves, thanks for the example too. I'll make a note and throw up a PR next week.
There was a problem hiding this comment.
Rindfleischetikettierungsüberwachungsaufgabenübertragungsgesetz
I see it's a real thing 🙃
There was a problem hiding this comment.
I'll make a note and throw up a PR next week
that's probably the point at which it doesn't need to be part of the hook.
I kept it in the hook for now to keep changes small as <BlockTitle /> (and therefore the legacy 35 char limit) is used very liberally.
talldan
left a comment
There was a problem hiding this comment.
Thanks for making those late changes.
This works really well, and I think it's also a nice little accessibility improvement that the label for the menu item now matches how the item in List View is announced.
|
In @ramonjd's absence, I'll merge this one in now since it looks like the main feedback has been addressed. We can always tweak further in follow-ups if need be, so let us know here if there was anything else folks wanted to see changed! |
Description
This PR employs a block display title in the block settings dropdown rather than the default core block name in the drop down label "Remove
xblock".A "block display title" might be a specific block variation title, or a reusable block group name.
This makes the label consistent with the block list view.
Changes
<BlockTitle />and turning it into a hook that we use independently of JSXBlockSettingsDropdown, which appears in the block settings toolbar and the block list view. This is to maintain (roughly) the dropdown container's current width proportions.35for all existing usages to maintain the status quo for now.Room for improvement
We might like to refactor
<Blocktitle />to use the<Text />component, which has truncation built in, e.g.,Also, maybe an aria-label with the full value might be appropriate where there is a truncation?
Testing Instructions
<BlockTitle />appear as they do on trunk (max 35 chars), e.g., bread crumb, block switcher...Run
npm run test-unit packages/block-editor/src/components/block-title/test/index.jsSome test code
Screenshots
Before
Remove a "Row" Block (a Group variation)
Remove a reusable block
After
Types of changes
Enhancement to the block settings dropdown.
Checklist:
*.native.jsfiles for terms that need renaming or removal).