Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Post Title Block: Add alignment and heading level support #22872

Merged
merged 6 commits into from
Jun 8, 2020
Merged

Post Title Block: Add alignment and heading level support #22872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
423d35a
Add support to the post title block for text alignment and heading le…
apeatling Jun 3, 2020
5b11a16
Fix up classname output, and add a default post title classname.
apeatling Jun 4, 2020
3332552
Add lightBlockWrapper, and __experimentalSelector to block.json.
apeatling Jun 5, 2020
dfed092
Fix block JSON trailing comma.
apeatling Jun 5, 2020
dee15ea
Fix PHP spacing lint errors.
apeatling Jun 5, 2020
48899c4
Update the core post title fixture.
apeatling Jun 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion packages/block-library/src/post-title/block.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,26 @@
"postId",
"postType"
],
"attributes": {
"align": {
"type": "string"
},
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think instead of handling the alignment manually, we could use the "align" support flag? Other blocks do that like "button".

Copy link
Contributor Author

@apeatling apeatling Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, we might want to add a support flag for text alignment later on.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it's super confusing. It should be textAlign.

"level": {
"type": "number",
"default": 2
}
},
apeatling marked this conversation as resolved.
Show resolved Hide resolved
"supports": {
"html": false
"html": false,
"lightBlockWrapper": true,
"__experimentalSelector": {
"core/post-title/h1": "h1",
"core/post-title/h2": "h2",
"core/post-title/h3": "h3",
"core/post-title/h4": "h4",
"core/post-title/h5": "h5",
"core/post-title/h6": "h6",
"core/post-title/p": "p"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👋 @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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prepared #24418 to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
}
53 changes: 51 additions & 2 deletions packages/block-library/src/post-title/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,32 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import {
AlignmentToolbar,
BlockControls,
__experimentalBlock as Block,
apeatling marked this conversation as resolved.
Show resolved Hide resolved
} from '@wordpress/block-editor';
import { ToolbarGroup } from '@wordpress/components';

export default function PostTitleEdit( { context } ) {
/**
* Internal dependencies
*/
import HeadingLevelDropdown from '../heading/heading-level-dropdown';
apeatling marked this conversation as resolved.
Show resolved Hide resolved

export default function PostTitleEdit( {
attributes,
setAttributes,
context,
} ) {
const { level, align } = attributes;
const { postType, postId } = context;
const tagName = 0 === level ? 'p' : 'h' + level;

const post = useSelect(
( select ) =>
Expand All @@ -20,5 +42,32 @@ export default function PostTitleEdit( { context } ) {
return null;
}

return <h2>{ post.title }</h2>;
return (
<>
<BlockControls>
<ToolbarGroup>
<HeadingLevelDropdown
selectedLevel={ level }
onChange={ ( newLevel ) =>
setAttributes( { level: newLevel } )
}
/>
</ToolbarGroup>
<AlignmentToolbar
value={ align }
onChange={ ( nextAlign ) => {
setAttributes( { align: nextAlign } );
} }
/>
</BlockControls>
Comment on lines +47 to +62
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated to this PR.

@diegohaz @ItsJonQ

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.

<Block
tagName={ tagName }
className={ classnames( {
[ `has-text-align-${ align }` ]: align,
} ) }
>
{ post.title }
</Block>
</>
);
}
14 changes: 13 additions & 1 deletion packages/block-library/src/post-title/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,19 @@ function render_block_core_post_title( $attributes, $content, $block ) {
return '';
}

return '<h1>' . get_the_title( $block->context['postId'] ) . '</h1>';
$tag_name = 'h2';
$align_class_name = empty( $attributes['align'] ) ? '' : ' ' . "has-text-align-{$attributes['align']}";

if ( isset( $attributes['level'] ) ) {
$tag_name = 0 === $attributes['level'] ? 'p' : 'h' . $attributes['level'];
}

return sprintf(
'<%1$s class="%2$s">%3$s</%1$s>',
$tag_name,
'wp-block-post-title' . esc_attr( $align_class_name ),
get_the_title( $block->context['postId'] )
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I have the logic here too #21420

Copy link
Contributor Author

@apeatling apeatling Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+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. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aduth had the logic for what you described in one of his PRs :)

It was in #18414 (specifically c7cded6).

Copy link
Contributor Author

@apeatling apeatling Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll give this one a go 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Started working on this here https://github.com/WordPress/gutenberg/pull/23007/files#diff-1334f2cb73e38f2f89d2aaf763f07ed9R307

}

/**
Expand Down
4 changes: 3 additions & 1 deletion packages/e2e-tests/fixtures/blocks/core__post-title.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,9 @@
"clientId": "_clientId_0",
"name": "core/post-title",
"isValid": true,
"attributes": {},
"attributes": {
"level": 2
},
"innerBlocks": [],
"originalContent": ""
}
Expand Down