Post Title Block: Add alignment and heading level support #22872
Conversation
apeatling
requested review from
ajitbohra,
jorgefilipecosta,
Soean and
talldan
as code owners
apeatling
added
[Block] Post Title
|
Size Change: +4.25 kB (0%) Total Size: 1.13 MB
ℹ️ View Unchanged
|
|
Converting to WIP/draft to add font size and color support before review. |
|
Based on #22843 (comment) will get this merged first and open a follow up PR. |
vindl
requested review from
youknowriad,
epiqueras,
noahtallen,
ockham,
vindl and
Addison-Stavlo
| "attributes": { | ||
| "align": { | ||
| "type": "string" | ||
| }, |
There was a problem hiding this comment.
Do you think instead of handling the alignment manually, we could use the "align" support flag? Other blocks do that like "button".
There was a problem hiding this comment.
The support flag adds block level alignment using floats though doesn't it? Other heading blocks use text alignment, so it seems more appropriate to replicate that for the post title block and provide better continuity.
There was a problem hiding this comment.
Yeah, we might want to add a support flag for text alignment later on.
There was a problem hiding this comment.
Ohh, I misunderstood this, I thought it was adding the text alignment support. Makes me wonder if we should rename the attribute or not? Do we already have blocks that support both text and block alignments? How do we name these things there?
There was a problem hiding this comment.
Yes, it's super confusing. It should be textAlign.
| $tag_name, | ||
| 'wp-block-post-title' . esc_attr( $align_class_name ), | ||
| get_the_title( $block->context['postId'] ) | ||
| ); |
There was a problem hiding this comment.
As an aside or a potential follow-up:
The "align" support flag automatically handles the addition of the className for static blocks (save functions), but it doesn't do so on the server. Now that we have server-side registration for most blocks, we could write some hooks to do this automatically. It involves some Regex work but I think it's doable. cc @gziolo
(same for other hooks like the custom className...)
There was a problem hiding this comment.
@aduth had the logic for what you described in one of his PRs :) The challenge is that we would make sure that the client and server always stay in sync. It's not only "align" and "className", but also colors, fonts, etc.
There was a problem hiding this comment.
Actually, I have the logic here too #21420
There was a problem hiding this comment.
+1 to supporting this, I'm looking to add color, font and line height support to the Site Title block and it needs server side support. 👍
There was a problem hiding this comment.
@youknowriad I've opened the Site Title block PR that would need support on the server side for the above: #23007
What needs to be done to get that support in master? I'm very happy to help with it.
There was a problem hiding this comment.
@apeatling I think right now the server side block registration already knows about the "supports" key (@gziolo confirm?), which means what's remaining is to write a render_block filter like that https://github.com/WordPress/gutenberg/pull/21420/files#diff-4339a9e86722fd029c231f3fb65f0ad2R392-R420 to apply the styles and classNames.
There was a problem hiding this comment.
I'll give this one a go 👍
There was a problem hiding this comment.
Started working on this here https://github.com/WordPress/gutenberg/pull/23007/files#diff-1334f2cb73e38f2f89d2aaf763f07ed9R307
|
Thanks for the reviews, I'll fix these things up. 👍 |
| <BlockControls> | ||
| <ToolbarGroup> | ||
| <HeadingLevelDropdown | ||
| selectedLevel={ level } | ||
| onChange={ ( newLevel ) => | ||
| setAttributes( { level: newLevel } ) | ||
| } | ||
| /> | ||
| </ToolbarGroup> | ||
| <AlignmentToolbar | ||
| value={ align } | ||
| onChange={ ( nextAlign ) => { | ||
| setAttributes( { align: nextAlign } ); | ||
| } } | ||
| /> | ||
| </BlockControls> |
There was a problem hiding this comment.
Unrelated to this PR.
We need to document the move from ToolbarGroup to Toolbar and what each Toolbar component does. Adding things to the Toolbar is one of the main parts of developing blocks, and right now, it's even hard for Core developers to figure out what the latest way of doing it is.
|
There's a failing test with this PR that I'm unclear from the failure on how to resolve:
@enriquesanchez or @youknowriad Any docs or pointers you can offer on this? Thanks. |
|
Thanks, I was looking in the wrong place. 👍 |
| "core/post-title/h4": "h4", | ||
| "core/post-title/h5": "h5", | ||
| "core/post-title/h6": "h6", | ||
| "core/post-title/p": "p" |
There was a problem hiding this comment.
👋 @apeatling I'm trying to understand why do we need the core/post-title/p here? It looks like this block behaves like the heading block: it can have 1-6 levels, but that's about it, right?
There was a problem hiding this comment.
I loosely recall there being support for using P as a selector in a previous version of the block, that I removed. This was left over. I might have that wrong but it was something like that. In any case, removing it makes sense.
Part of #21087
Description
Adds text alignment and heading level support to the Post Title block. One question is how to handle the shared code for the
HeadingLevelDropdowncomponent. I expect this needs to be extracted from the heading block so it can be shared appropriately.How has this been tested?
Screenshots
Types of changes
New feature.
Checklist: