List View: Use heading content for button label text if available #41855
Conversation
andrewserong
added
[Type] Enhancement
packages/block-editor/src/components/list-view/block-select-button.js
Outdated
Show resolved
Hide resolved
|
Size Change: +266 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
There's an existing system for doing this that should make the PR a bit smaller. You can see how the navigation link block achieves it here - https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/navigation-link/index.js#L25. |
|
@talldan that reminds me, we should probably unexperimentalize that API :) |
|
Thanks @talldan, using that function's a great idea! I've updated this PR — I went a little further and added a |
andrewserong
changed the title
I wonder if that'll be needed. I think the only other place it'll display is the breadcrumb at the bottom of the screen. I think other blocks that use It might be good to get input from a designer about this part. |
Good question, it appears to be used for both the breadcrumb at the bottom of the screen and the Remove button in the dropdown (the settings dropdown uses
I'll add a "Needs Design feedback" label. TL;DR for designers: should we use Heading content as the label just in the list view, or is it also okay in the breadcrumb at the bottom of the screen, and in the remove button in the settings menu? (I'm happy to implement it either way 🙂) |
Hmm, yeah, it doesn't look great in the remove button. There may need to be some more nuance in how this is shown. I think the distinction is in whether the title shows content (like it will for Heading and does for Navigation Link), or whether it represents the semantic name of a block (like it does for variations and the names of template parts / reusable blocks). |
Ah, so we could potentially use a context value of |
|
I think the problem with really specific terms like 'list-view' is that this could grow into a really big list of contexts as different types of UI are introduced, or ideas around this are iterated on. All in all, I'm not sure this context system is the right solution in the first place. Maybe it should be something like this pseudocode: getLabels( blockType, attributes ) {
return {
// used where there's a short amount of space for the block (e.g. remove block button)
title: `${ blockType.title } (${ attributes.level })`,
// used where there's more space (list view, breadcrumbs)
label: attributes.content,
// used for aria labels
accessibility: `${ attributes.level } - ${ attributes.content }`,
};
}These outstanding issues are one of the reasons why it hasn't been stabilised yet |
Ah, gotcha. And then it's also potentially tricky if we want different behaviour for different blocks (like in the screenshot of how the Paragraph block might work: #41855 (comment)), which appears to be a strength of the existing API 🤔. If we wind up wanting to proceed with the label in the list view being different from the remove button or breadcrumbs, but it's too fiddly to land on a change to that API, I can always roll this PR back to the initial commit. Alternately, if it's acceptable that the Remove button also uses the content, then there's a clearer path to a smaller changeset for this PR (just change the Thanks again for thinking through the implications, and I'm happy to keep hacking away tomorrow! |
|
My inclination would be to use only on list-view. Not quite sure about breadcrumbs, but I lean towards no. |
|
Thanks for the feedback! I ran out of time to progress this one today, but I'll revisit next week. I suspect we'll need a little more logic to make sure that long titles truncate comfortably for headings with a custom anchor That said, do let me know if anyone feels strongly about how it should be implemented, though, and I'll incorporate the feedback when I revisit this next week. Thanks, all! 🙇 |
|
I've updated this PR to revert back to the approach of using some hard-coded logic in the I took a look at how to get the truncation to play nicely with the
In the above screenshot, from the highlighted Heading:
In each of the above examples, space is effectively "reserved" for the lock icon via the max width settings. If the lock is removed on that particular heading, then the whole List View returns to the default width of To achieve the above, I've added a The PR probably still needs a little bit of tidying, but happy to try implementing any other ideas. I think the additional (Note: I think the current change possibly breaks the e2e tests, so I'll look at fixing that up, too) |
Sorry to potentially make you u-turn again, but I do think it'd be better to use the existing label API. The I don't think defining the block names implicitly in list view is the way to go - it wouldn't be something that could easily develop as an API because it limits the feature to core blocks. |
Not a bother at all @talldan, it's easy to revert! I'll rework this PR to go with the |
|
Just a note for myself, I think the e2e tests are failing because of the additional |
|
From testing just now - anchor text truncates a little early and doesn't show many characters: Before: After: It feels like it should take up any available space. It's probably really tricky to account for all the different situations. |
It sure is! There's kind of a delicate balancing act between which things should fill up which available space and when 😆 |
There was a problem hiding this comment.
Works really well! Thanks for sticking with this.
I left an earlier comment about the anchor. If it's an easy fix to improve that before merging, it'd be great. I think it should be possible through some flexboxery.
If it's complicated it might be worth leaving it to a separate PR.
…out anchor id text
… descendent of the anchor tag instead of an immediate child
… the block title sits in the DOM hierarchy
…Blocks still returns the anchor elements, and not the child span nodes
…ctedly extending the panel
…block contents wrapper
…hor to ensure it never steals more than half of the row width
andrewserong
force-pushed
the
try/use-heading-content-as-label-in-list-view
branch
from
eeb6705 to
26d7799
Compare
|
Thanks for the reviews, folks! It turned out not to be too hard to fix up the anchor, too — I wound up using Seems to be working nicely now, so I'll merge this in once tests pass:
Happy to continue tweaking in follow-ups, of course — I'll have a go at the Paragraph block next. Thanks again! 🙇 |
|
We could consider not rendering the anchor label on headings when they match the same name. I also think it might be useful to add a "Copy anchor" to the ellipsis menu. |
What?
Explore conditionally using the Heading block's content as the block button label in the List View, falling back to the block's title if no content is available.
Why?
As discussed in #33583, this could help improve navigating around the List View, and make it feel a little more like working with a document outline.
How?
BlockTitlecomponent anduseBlockDisplayTitlehook to support passing in acontextstring that will then be passed along togetBlockLabel.list-viewcontext string and then conditionally return the block's content as the label.list-viewas the context toBlockTitlefor the block button.Testing Instructions
Screenshots or screencast
In the below screengrab, the empty Heading block defaults to using the block name as the label. As the content is updated, the label updates to use the block's content.