Use blockLabel for BlockTitle component #23847
Conversation
noahtallen
added
[Type] Enhancement
noahtallen
requested review from
ockham,
youknowriad,
mtias,
vindl,
Copons,
epiqueras and
Addison-Stavlo
|
Size Change: +77 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
The block title is used in a lot of places. Have you checked that it makes sense to swap in the label everywhere? cc @youknowriad |
|
Searching through the repo, I see the component is used in fewer places than I expected:
The only concern I'd have is if other blocks would not want their display name/label used in these UI. But I'm not sure what the point of the label is if it wouldn't be used in these cases 🤔 |
|
Thanks for this. I think the change makes sense. Let's see what Riad thinks. |
|
I think this could be a bit challenging applied widely. For example, it affects the navigation menu in ways that may not be desirable:
While the sidebar inspector remains as:
|
I agree that the breadcrumb there would become unusable. Back when the block label was introduced, there was a discussion about optionally displaying the block name as well, which was shelved for overcomplicating things. |
|
If that's the case, I'm not sure what the best solution is here. 🤔 Thinking through some options & ideas: Don't we want to use the nav link's label in the breadcrumb for the same reason we want the template part's label there? In prod it looks like this: With plain label it looks like this: What if we updated the label to be like this or If we don't do that, it sounds like the label is not a good fit for the breadcrumb, in which case we either have to invent a new API (which doesn't seem like a good idea to me). Or add more settings to the
We could also change the sidebar inspector to use the block label. I'm not sure we want to do that 🤔 But at the same time, I don't search "ocean" or "header" in the block inserter, I search "link" and "template part" (so the proper block title is still needed in some cases) cc @talldan if you have ideas :) |
|
|
|
A related question is do we want that to be the raw label in general, or do we want to make the |
|
The latter for flexibility I think. |
|
|
|
Reading through this PR I think it is a very good idea using labels in the bottom breadcrumbs area. Long labels could be truncated, as we do not need to know all the text contained in the label. |
@noahtallen This suggestion from @epiqueras makes sense, but good to get more designer eyes on this. As part of that we should make sure that screenreaders don't end up with a label that only has partial words from the text truncation (like the one quoted above). Let's make sure a round of accessibility testing happens on the changes. |
|
I thought at one point we were possibly getting rid of the breadcrumbs? Maybe that's not the case anymore. Of the options listed here, I do prefer the parenthesis. It provides much more clarity beyond the typical block type AND the parenthesis provide a better visual indication of a label than the Another possible route is using colons.
|
At least, in FSE, it is currently one of the few indicators of what you're editing. That's a broader problem, but it is useful in that context right now
I'm happy to use any separator we want to go with. How many characters would we like to truncate to? |
|
cc @shaunandrews @MichaelArestad I'd like to move forward with this issue, but I think I'm still confused which direction to go. It sounds like we could use the current approach of the PR, which definitely solves the original "problem". Or we could go with the "tooltip" method. I think I prefer the current approach, but it really comes down to: Is a user going to expect to see "header" or "template part"? In my mind, many end-users might expect "header", but maybe theme devs would expect "template part". |
|
Could we have a "template part: header"? |
|
yeah, that should be working in the current code on this branch: #23847 (comment) |
|
A tooltip is harder to discover. As it requires the user to hover the object that shows the tooltip. |
@paaljoachim I'm not sure I agree with that. It's not clear to me that the added label is useful enough to be always visible in the breadcrumbs. My concern lies with clarity. Breadcrumbs are useful for getting an understanding of the current hierarchy and an okay way to navigate to a parent. Does adding that label add clarity? If the issue is around concern of which template part the user is modifying, that's being explored here. So far the proposals are more visible than any change we make to the breadcrumb bar.
@noahtallen I don't think either are going to spend much time looking at the breadcrumb bar. I lean toward consistency when looking at problems like this. I would go with the block title to be consistent with the rest of the items in the breadcrumbs. I like the idea of exposing the name or other added identifying information in a tooltip. Yes, it's not always visible, but, in most cases, I don't think it needs to be. This is not something that has a clear answer to me. Due to the location and visibility of the breadcrumbs bar, we can try either way. If you want to try using the block names there, I think we can probably try that for a bit in master to see how it feels. |
I think the template part visibility issues is compounded by lots of smaller issues. Improving that means improving lots of small things which might include the breadcrumb :)
I think that the label does add clarity if the user is using the breadcrumb. "Template Part" is not a concept that many end-users will understand or know about without doing some research or learning. I think anything we can do to help guide that sort of user to understand the concept of template part better would be very nice. I definitely understand your argument about consistency, and I think that's really important. In my opinion, we could use the label in more places as well to improve consistency (rather than just the title). I think there are likely many dynamic blocks which could benefit from better labels in more places (like the nav items, and probably lots of the other FSE blocks dealing with dynamic values)
I like this idea myself. The code changes here are minimal, so we can easily iterate as needed! |
noahtallen
force-pushed
the
try/use-block-label-for-breadcrumb
branch
from
25878f6
to
1146683
Compare
|
Here is a gif of the current UI (don't mind the gibberish titles =))
|
|
I might have missed an update in one of the threads, but to clarify, are we supporting special characters? The discussion here seems to imply that they were previously working. I am, however, having trouble rendering them locally.
Everything else works as described in Firefox, Chrome, Edge, Safari, and IE11. 🚀 +1 from me when we resolve our discussion about special characters 🙂 |
|
I really like seeing the selected block/object name listed in the breadcrumbs. As it really gives a nice quick overview. I can just quickly glance at the breadcrumbs and right away see where I am in the hierarchy and what is selected. |
@jeyip Ah, sorry about that, I was only testing the Lodash |
I didn't really think about it, but ideally they would work. Unless special characters are unsupported by the template part slug/name field in the database. This is probably an issue lower in the stack.... maybe something in the labels? I'll look into it though! |
Co-authored-by: Pascal Birchler <pascalb@google.com>
noahtallen
force-pushed
the
try/use-block-label-for-breadcrumb
branch
from
d7cfc71
to
60cd2e5
Compare
|
The reason that emojis are excluded is because we use
|
|
I think it basically needs to be sanitized into something which works as a post slug. (i.e. it's not really a display name.) |
|
The title also won't work because it gets cleaned in a similar way. I think we can stick with the current behavior now as it is expected and doesn't change anything. That said, there has been some discussion about introducing proper "labels" for template parts. (#21161 (review)) |
|
I'll move forward with merging this. I am happy to address any further feedback in follow-ups! |
|
Nice work @noahtallen |
|
I should clarify this:
This actually isn't true, my test code was just incorrect. But there are still other issues with using the post title as the label that would make it fit for a follow-up PR :) |
Description
Changes the
BlockTitlecomponent to use the block label instead of just the title. (Note that the label will fall back to the title if no label is set.)Fixes #23463
How has this been tested?
Locally, in edit-site. You can verify the change by looking for a computed label in the block breadcrumb.
Screenshots
Types of changes
Enhancement
Checklist: