Blocks: Refactor blocks to use supports align #5099

gziolo · 2018-02-16T13:01:13Z

Description

Follow up for #4069.

This PR seeks the way to refactor all existing core block to use newly introduced align option for inside supports field. It also adds missing changes to the documentation.

I skipped 3 4 blocks which use non-standard properties:

image
paragraph
text-columns
latests-posts

We may leave it as it is or we can tackle in another PR.

How to test

For testing purpose it makes sense to enable wide alignment as described here.

Verify that there are no regressions in the alignment behavior of the following blocks:

button
categories
columns
cover image
embed
gallery
~~latest posts~~ - removed from this PR, see: Blocks: Refactor blocks to use supports align #5099 (comment)
pullquote
table
video

Ensure unit tests pass:

npm test

Types of changes

Refactoring.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code has proper inline documentation.

gziolo · 2018-02-16T14:06:09Z

blocks/hooks/align.js

@@ -24,7 +24,7 @@ import { getBlockSupport, hasBlockSupport } from '../api';
 * @return {Object}          Filtered block settings
 */
 export function addAttribute( settings ) {
-	if ( hasBlockSupport( settings, 'align' ) ) {
+	if ( hasBlockSupport( settings, 'align' ) && ! has( settings, [ 'attributes', 'align' ] ) ) {


It would be nice to avoid this workaround. I added to make sure that tests pass, which gave me an impression that it might trigger some warnings in the browser when loading old posts.

The failing tests I'm seeing when removing this workaround seem legitimately concerning though?

Expected: <blockquote class=\"wp-block-pullquote alignundefined\"

Why would alignundefined be expected?

Worth looking into...

Some of the failures could also be cleared up by running npm run fixtures:regenerate (specifically, the _domReact key changes)

gziolo · 2018-02-16T14:07:28Z

blocks/library/columns/index.js

 	edit( { attributes, setAttributes, className, focus } ) {
-		const { align, columns } = attributes;
+		const { columns } = attributes;
 		const classes = classnames( className, `has-${ columns }-columns` );

 		return [
 			...focus ? [


@youknowriad should we replace ...focus with isSelected?

Updated 👍

gziolo · 2018-02-16T14:10:15Z

docs/block-api.md

@@ -127,36 +127,52 @@ useOnce: true,

 * **Type:** `Object`

-Optional block extended support features. The following options are supported, and should be specified as a boolean `true` or `false` value:
+Optional block extended support features. The following options are supported, and should be specified as a boolean `true` or `false` value in most of the cases:


Not sure how to better phrase this exception for an array :)

Maybe avoid the second part of the sentence, just leaving:

Optional block extended support features. The following options are supported:

aduth · 2018-02-17T00:26:10Z

blocks/library/columns/index.js

-						} }
-					/>
-				</BlockControls>,
+			isSelected ? [


Since there's only a single entry, this could be simplified to:

isSelected && (

Also dropping the key from InspectorControls

aduth · 2018-02-17T00:29:22Z

docs/block-api.md

@@ -127,36 +127,52 @@ useOnce: true,

 * **Type:** `Object`

-Optional block extended support features. The following options are supported, and should be specified as a boolean `true` or `false` value:
+Optional block extended support features. The following options are supported, and should be specified as a boolean `true` or `false` value in most of the cases:


Maybe avoid the second part of the sentence, just leaving:

Optional block extended support features. The following options are supported:

aduth · 2018-02-17T00:30:12Z

blocks/library/pullquote/index.js

-					/>
-				</BlockControls>
-			),
+		return (
 			<blockquote key="quote" className={ className }>


key unnecessary.

aduth · 2018-02-17T00:30:21Z

blocks/library/table/index.js

-				</BlockControls>
-			),
+
+		return (
 			<TableBlock
 				key="editor"


key unnecessary.

aduth · 2018-02-17T00:30:57Z

docs/block-api.md

 html: false,
 ```

+- `wideAlign` (default `true`): Gutenberg allows to enable [wide alignment](themes.md#wide-alignment) for your theme. To disable this behavior for a single block, set this flag to `false`.


This link won't work when ported to the WordPress.org handbook. It needs to be an absolute URL.

aduth · 2018-02-17T00:31:11Z

docs/block-api.md

 html: false,
 ```

+- `wideAlign` (default `true`): Gutenberg allows to enable [wide alignment](themes.md#wide-alignment) for your theme. To disable this behavior for a single block, set this flag to `false`.
+
+```js


Missing the closing ```

aduth · 2018-02-17T00:32:56Z

blocks/library/embed/index.js

@@ -186,8 +166,7 @@ function getEmbedBlockSettings( { title, icon, category = 'embed', transforms, k
 					typeClassName += ' is-video';
 				}

-				return [
-					controls,
+				return (
 					<figure key="embed" className={ typeClassName }>


key unnecessary.

aduth · 2018-02-17T00:33:05Z

blocks/library/embed/index.js

 				}

 				if ( ! html ) {
 					const label = sprintf( __( '%s URL' ), title );

-					return [
-						controls,
+					return (
 						<Placeholder key="placeholder" icon={ icon } label={ label } className="wp-block-embed">


key unnecessary.

aduth · 2018-02-17T00:37:34Z

blocks/library/button/index.js

-		const props = {};
+	supports: {
+		align: true,
+		wideAlign: false,


Why wideAlign: false? Seems to be allowed in master.

It's broken in master though :) Styles aren't applied for full and wide properly... We can fix that.

I don't think it is designed to work with the theme option alignWide.

By the way, we should rename wideAlign to alignWide to match with PHP.

This is how wide looks like in action:

Let's leave it disabled for the time being. @karmatosed @jasmussen any plans to make button block wide aligned?

jasmussen · 2018-02-19T14:01:47Z

Can't find the particular discussion now, the deeplink didn't work. But a question was asked: should the button support wide alignments? The answer is probably not. The button is good for centering and floating, but it is likely to shine once it can live as a nested block inside containers. At that point you might start to create some reusable blocks that could feature perhaps an image, some text, and a button, all nested inside a single "box" block or whatever we end up calling it. This box block would then support wide alignments, and the content inside would scale to that. But it doesn't seem like there's a huge value in having these alignments on the button itself.

gziolo · 2018-02-19T14:46:34Z

@jasmussen, thanks for the confirmation. @aduth this is ready for another check. I addressed all comments.

aduth · 2018-02-19T20:29:23Z

blocks/hooks/align.js

@@ -24,7 +24,7 @@ import { getBlockSupport, hasBlockSupport } from '../api';
 * @return {Object}          Filtered block settings
 */
 export function addAttribute( settings ) {
-	if ( hasBlockSupport( settings, 'align' ) ) {
+	if ( hasBlockSupport( settings, 'align' ) && ! has( settings, [ 'attributes', 'align' ] ) ) {


The failing tests I'm seeing when removing this workaround seem legitimately concerning though?

Expected: <blockquote class=\"wp-block-pullquote alignundefined\"

Why would alignundefined be expected?

Worth looking into...

aduth · 2018-02-19T20:29:46Z

blocks/library/pullquote/index.js

-			</blockquote>,
-		];
+			</blockquote>
+		);
 	} ),

 	save( { attributes } ) {


The save implementation should no longer be responsible for applying the align class.

Same goes for all other refactored blocks.

I missed that, good catch 👍

aduth · 2018-02-19T20:31:22Z

blocks/hooks/align.js

@@ -24,7 +24,7 @@ import { getBlockSupport, hasBlockSupport } from '../api';
 * @return {Object}          Filtered block settings
 */
 export function addAttribute( settings ) {
-	if ( hasBlockSupport( settings, 'align' ) ) {
+	if ( hasBlockSupport( settings, 'align' ) && ! has( settings, [ 'attributes', 'align' ] ) ) {


Some of the failures could also be cleared up by running npm run fixtures:regenerate (specifically, the _domReact key changes)

gziolo · 2018-02-20T20:29:53Z

blocks/library/latest-posts/index.php

@@ -92,7 +92,6 @@ function register_block_core_latest_posts() {
 			),
 			'align'           => array(
 				'type'    => 'string',
-				'default' => 'center',


This one is tricky because we set all attributes using PHP code, but then override with JavaScript using hook. Should we add deprecation block for this case? /cc @youknowriad

gziolo · 2018-02-20T20:30:53Z

blocks/library/text-columns/test/__snapshots__/index.js.snap

@@ -2,7 +2,7 @@

 exports[`core/text-columns block edit matches snapshot 1`] = `
 <div
-  class="wp-block-text-columns alignundefined columns-2"
+  class="wp-block-text-columns columns-2"


alignundefined doesn't make too much sense, that's why used classnames to fix this.

gziolo · 2018-02-20T20:32:13Z

blocks/hooks/align.js

@@ -131,7 +131,7 @@ export function withAlign( BlockListBlock ) {
 export function addAssignedAlign( props, blockType, attributes ) {
 	const { align } = attributes;

-	if ( includes( getBlockValidAlignments( blockType ), align ) ) {
+	if ( align && includes( getBlockValidAlignments( blockType ), align ) ) {


I think this is where alignundefined could be generated before this commit.

But... how? Looking through getBlockValidAlignments, I can't envision any scenario where it should return an array with undefined as an entry.

And if it does, that sounds like a bug with getBlockValidAlignments.

Ooops, you are right. It is obsolete unless we want to add performance optimization to avoid array lookup :P I'm removing this change.

gziolo · 2018-02-20T20:32:43Z

I regenerated tests and applied a few tweaks with a3ceee5.

aduth · 2018-02-21T23:55:18Z

blocks/hooks/align.js

@@ -131,7 +131,7 @@ export function withAlign( BlockListBlock ) {
 export function addAssignedAlign( props, blockType, attributes ) {
 	const { align } = attributes;

-	if ( includes( getBlockValidAlignments( blockType ), align ) ) {
+	if ( align && includes( getBlockValidAlignments( blockType ), align ) ) {


But... how? Looking through getBlockValidAlignments, I can't envision any scenario where it should return an array with undefined as an entry.

And if it does, that sounds like a bug with getBlockValidAlignments.

aduth · 2018-02-22T00:50:15Z

blocks/library/button/index.js

@@ -67,18 +65,12 @@ class ButtonBlock extends Component {
 			text,
 			url,
 			title,
-			align,
 			color,
 			textColor,
 			clear,
 		} = attributes;

 		return [


This no longer needs to be returned as an array.

There is an optional form in line 117: https://github.com/WordPress/gutenberg/blob/a3ceee55afbe0200a1567945e14de6f3b3f14f61/blocks/library/button/index.js#L117. It only seems that you can omit array structure here :)

aduth · 2018-02-22T00:50:25Z

blocks/library/button/index.js

-				<BlockControls key="controls">
-					<BlockAlignmentToolbar value={ align } onChange={ this.updateAlignment } />
-				</BlockControls>
-			),
 			<span key="button" className={ className } title={ title } ref={ this.bindRef }>


And this will no longer need key when not returned as part of an array.

See my previous comment.

aduth · 2018-02-22T00:52:24Z

blocks/library/button/index.js

-			props[ 'data-align' ] = align;
-		}
+	getEditWrapperProps( { clear } ) {
+		const props = {};


Since getEditWrapperProps can return undefined, this could potentially be simplified to:

getEditWrapperProps( { clear } ) { if ( clear ) { return { 'data-clear': 'true' }; } }

aduth · 2018-02-22T00:53:05Z

blocks/library/button/index.js

@@ -210,7 +198,7 @@ export const settings = {
 		const linkClass = 'wp-block-button__link';

 		return (
-			<div className={ `align${ align }` }>


This is the only unfortunate thing about these "magic" supports. It becomes very non-obvious why we need the div wrapper at all.

Yes, this might have up to 3 class names inserted in here :)

aduth · 2018-02-22T00:54:08Z

blocks/library/button/index.js

-		attributes: blockAttributes,
+		attributes: {
+			...blockAttributes,
+			align: {


Since a deprecated entry supports supports, could we just include align there?

I don't think it will work. The deprecated entry doesn't copy attributes, supports and save: ...omit( blockType, [ 'attributes', 'save', 'supports' ] ). Hooks we have with the current implementation don't modify the deprecated block types. @youknowriad can you confirm if the current implementation is valid?

aduth · 2018-02-22T00:55:28Z

blocks/library/columns/index.js

-				</BlockControls>,
-				<InspectorControls key="inspector">
+			isSelected && (
+				<InspectorControls>


I might have made the opposite claim previously, but since this is now part of the top-level return array, won't this need a key?

Yes, it needs :)

aduth · 2018-02-22T01:01:11Z

blocks/library/gallery/index.js

 		return (
-			<ul className={ `align${ align } columns-${ columns } ${ imageCrop ? 'is-cropped' : '' }` } >
+			<ul className={ `columns-${ columns } ${ imageCrop ? 'is-cropped' : '' }` } >


Unrelated to your changes, but this is begging for classnames 😄

Let's add in this PR this change, too 👍

aduth · 2018-02-22T01:02:32Z

blocks/library/latest-posts/index.php

@@ -91,8 +91,7 @@ function register_block_core_latest_posts() {
 				'default' => 3,
 			),
 			'align'           => array(
-				'type'    => 'string',
-				'default' => 'center',
+				'type' => 'string',


Oof... did we decide anything about how we're going to handle server attributes for supports properties, particularly if we're planning for server-registered attributes to become the canonical set?

I think I will revert all changes for the latest posts block, as this is falls under special case group :) We can tackle it later.

So this one is tricky. It is exposed with the global variable and consumed when the block is registered, but it is not that easy to ensure it behaves the same in the JS and PHP context. We might want to revisit the idea of using has check as I proposed earlier: #5099 (comment):

if ( hasBlockSupport( settings, 'align' ) && ! has( settings, [ 'attributes', 'align' ] ) ) {

I think I will revert all changes for the latest posts block, as this is falls under special case group :) We can tackle it later.

I think we ought to at least have a strategy, because I'm starting to worry that these supports which add attributes may be non-viable altogether. Or do you think it's just a matter of duplicating the logic in the server?

The issue here is that you need to define this attribute on the server to make it exposed to the save function when server-side rendering your block. This is a general issue with such blocks, there is lots of duplication. I think we should tackle all similar issues as a whole, not necessarily as part of this refactoring.

One way of dealing with it would be to mark a component as handled server-side. However it still doesn't solve the issue of code duplication, because we end up in a situation where edit is handled client-side, but save mostly server-side. I don't think we can completely switch over to the solution where this is cleanly separated. We can only mitigate the unexpected side-effects where we set something on the client but it isn't passed back to the server.

I think we should tackle all similar issues as a whole, not necessarily as part of this refactoring.

I'm personally considering it a blocker because I'm not convinced yet there's a solution at all.

@youknowriad, @mtias or @mcsf, any ideas how to unblock it? :) The only thing I can come up is to avoid adding align to attributes if it was already provided server-side, but it doesn't solve the root of the problem. We need to make some decisions how do we handle server-side rendered blocks. The main question is how much duplication are we happy to introduce between PHP and JS?

gziolo · 2018-02-22T11:27:23Z

As discussed in #5099 (comment), I removed all changes to the latest posts block from this PR. This PR is big enough so let's tackle all 4 remaining blocks that have a non-standard block alignment in the follow-up PRs.

I rebased with master and addressed all comment. I plan to merge it tomorrow, if you agree.

gziolo · 2018-04-24T07:11:15Z

It's on hold since we didn't agree how to tackle server-side rendered blocks.

gziolo · 2018-05-02T08:07:04Z

Closing this one as it became stale and would require lots of work to make it up to date, given all the changes introduced in the meantime. We still didn't come up with a solution how to tackle server-side rendered blocks. In addition, there were another smaller PRs which added align support for the individual blocks (like #6364) or other where we remove support for align at all (like #6511)...

gziolo added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress labels Feb 16, 2018

gziolo self-assigned this Feb 16, 2018

gziolo requested a review from aduth February 16, 2018 13:06

gziolo force-pushed the update/blocks-supports-align branch 4 times, most recently from cdd5657 to 3d51715 Compare February 16, 2018 14:02

gziolo commented Feb 16, 2018

View reviewed changes

gziolo added [Feature] Extensibility The ability to extend blocks or the editing experience and removed [Status] In Progress Tracking issues with work in progress labels Feb 16, 2018

gziolo force-pushed the update/blocks-supports-align branch from 3d51715 to 5c261c4 Compare February 16, 2018 14:36

aduth reviewed Feb 17, 2018

View reviewed changes

gziolo force-pushed the update/blocks-supports-align branch from 5c261c4 to 2823dab Compare February 19, 2018 13:46

aduth previously requested changes Feb 19, 2018

View reviewed changes

aduth mentioned this pull request Feb 19, 2018

Fixed a bug in paragraph affecting content writing when block is left/right aligned. #5135

Merged

gziolo force-pushed the update/blocks-supports-align branch from 2680225 to a186cdf Compare February 20, 2018 20:27

gziolo commented Feb 20, 2018

View reviewed changes

gziolo force-pushed the update/blocks-supports-align branch from a186cdf to a3ceee5 Compare February 20, 2018 20:35

aduth reviewed Feb 22, 2018

View reviewed changes

gziolo mentioned this pull request Feb 22, 2018

Adjust alignment-toolbar so that we disable the left, middle & right alignment options as needed #5195

Closed

gziolo force-pushed the update/blocks-supports-align branch 2 times, most recently from cbf452f to 04e670b Compare February 22, 2018 11:24

gziolo added 7 commits February 28, 2018 08:31

Blocks: Refactor blocks to use supports align

b4ebda2

Block: Code cleanup after review for supports align feature

baaa079

Blocks: Rename wideAlign option to alignWide for consistency

9b1d5ae

Blocks: Remove obsolete key props in embed block

e19d12b

Blocks: Refactor tests to work with new supports align option

ddad84d

Blocks: More refactoring for blocks to work with supports align

3a32e1c

Blocks: Revert supports align changes for the latest posts block

f903bb0

gziolo force-pushed the update/blocks-supports-align branch from 04e670b to f903bb0 Compare February 28, 2018 09:18

jeffpaul mentioned this pull request Mar 15, 2018

Alignment is handled inconsistently across blocks #4010

Closed

jeffpaul added this to the Merge Proposal milestone Mar 15, 2018

aduth mentioned this pull request Mar 16, 2018

Issue/5644 Adding custom CSS support in Latest Posts Block #5649

Closed

2 tasks

gziolo mentioned this pull request Mar 18, 2018

[WIP] Blocks: Explore how we could divide server and client block properties #5652

Closed

3 tasks

jvisick mentioned this pull request Mar 20, 2018

Issue/5612 - Alignfull class not working on columns #5704

Closed

3 tasks

karmatosed modified the milestones: Merge Proposal, Merge Proposal: Plugins Apr 12, 2018

gziolo mentioned this pull request Apr 24, 2018

Used align hook in columns block; Fixed columns alignment bug; #6364

Merged

gziolo closed this May 2, 2018

gziolo deleted the update/blocks-supports-align branch May 2, 2018 08:07

gziolo mentioned this pull request May 16, 2018

Block API: Introduce block definitions and implementations #6733

Closed

designsimply added the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Jul 11, 2018

aduth mentioned this pull request Sep 13, 2018

Block API: Server-side awareness of block types #2751

Closed

Blocks: Refactor blocks to use supports align #5099

Blocks: Refactor blocks to use supports align #5099

Conversation

gziolo commented Feb 16, 2018 • edited Loading

Description

How to test

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasmussen commented Feb 19, 2018

gziolo commented Feb 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Feb 20, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Feb 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo commented Feb 22, 2018

gziolo commented Apr 24, 2018

gziolo commented May 2, 2018 • edited Loading

gziolo commented Feb 16, 2018 •

edited

Loading

gziolo commented Feb 20, 2018 •

edited

Loading

gziolo Feb 22, 2018 •

edited

Loading

gziolo Feb 22, 2018 •

edited

Loading

gziolo Feb 22, 2018 •

edited

Loading

gziolo commented May 2, 2018 •

edited

Loading