Editor: Deprecate `layout` attribute #11029

aduth · 2018-10-24T21:23:49Z

Related: #7234
Related: #7841

This pull request seeks to deprecate layout-related behaviors. As of #7234, no core functionality leverages this feature. Its need has been superseded by the additional functionality of block insertion restrictions via allowedBlocks and parent. It's continued existence has already proven to be a maintenance burden, the removal of which (not done here) will enable a significant simplification to the editor implementation.

Testing instructions:

There should be no deprecated warnings when using the editor, since no functionality exists leveraging layouts.

Verify there are no regressions (nor warnings) in the behavior of drag-and-drop.

Verify that an InnerBlocks implementation using layouts still works, but logs warnings (needs example).

gziolo · 2018-10-25T05:03:38Z

I just wanted to ask for confirmation if it has nothing to do with layout hooks (https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/hooks/layout.js)?

chrisvanpatten · 2018-10-25T12:16:45Z

I wrote a quick layout-based block (sorry, it's ES6 🙈) but it isn't working right. The layouts aren't created in the editor and layout classes are definitely not rendered onto each block, the way they were in the past. I have to guess this is due to changes in InnerBlocks. It's also possible I made a dumb mistake but I checked in a few places and I am fairly sure my code is correct.

However… it's also not working in master. I would guess this is a regression from a while back that nobody noticed.

All the more reason to ditch it!

With that said, since layouts in their current form are so broken, is it even worth considering just cutting the code without a deprecation?

/**
 * WordPress Dependencies
 */
import { __ } from '@wordpress/i18n';
import { registerBlockType } from '@wordpress/blocks';
import { Fragment } from '@wordpress/element';
import { InnerBlocks } from '@wordpress/editor';

/**
 * Register block
 */
registerBlockType('tomodomo/layout', {
  title: 'Layout Test',
  description: 'Test of custom layouts',
  keyword: [
    'layout',
  ],
  icon: {
    background: '#963f69',
    foreground: '#FFFFFF',
    src: 'columns',
  },
  category: 'layout',
  attributes: {},
  edit: (props) => {
    return (
      <Fragment>
        <InnerBlocks
          layouts={{
            layout1: {
              label: 'Layout 1',
              icon: 'columns',
            },
            layout2: {
              label: 'Layout 2',
              icon: 'columns',
            },
          }}
        />
      </Fragment>
    )
  },
  save: (props) => {
    return (
      <div>
        <InnerBlocks.Content />
      </div>
    )
  },
  supports: {
    align: [
      'wide',
      'full',
    ],
  },
})

aduth · 2018-10-25T12:28:52Z

I just wanted to ask for confirmation if it has nothing to do with layout hooks

It is related, yes. That entire file would be removed.

However… it's also not working in master. I would guess this is a regression from a while back that nobody noticed. All the more reason to ditch it!

This is one of the bigger reasons for removing it. Since it's so rarely used (and completely unused in core) it's all the more likely to be prone to breakage. Not to say that can't be guarded against with tests, but all the hypotheticals around the value of this feature have really not panned out, and it's becoming more of a time sink to support.

chrisvanpatten

Left some notes in a comment, but it seems like layouts may have actually been broken for some time. All the more reason to ditch, and perhaps ditch without deprecation.

That would be an exception to our normal approach, but in this case I have a hard time imagining anyone is using layouts in a meaningful way (unless they're on a very old version of Gutenberg) considering that they just don't seem to work.

I'm not 100% comfortable commenting on the code in the PR necessarily, but those are my thoughts from a former heavy layouts user :)

EDIT: Somehow missed your reply to my comment @aduth. Sorry! But yes, I agree, let's ditch it.

youknowriad · 2018-10-29T09:02:54Z

Since it's a broken API for now, can't we just remove it instead of deprecating it?

aduth · 2018-10-29T12:30:34Z

I'm okay with that. I wasn't aware just how non-functional they were. I'll revise this pull request today.

This reverts commit c7d28be.

aduth · 2018-10-29T14:03:23Z

Rebased and pushed with new approach which removes layout altogether.

A few notes:

The simplifications afforded are quite nice, as expected.
I want to test (perhaps programmatically) the migration of the old Columns implementation which had used layouts, as with the removal of the hooks, this attribute is no longer added to any block. At worst, we may need to artificially reintroduce the layout attribute for the deprecated column.
Technically there are undeprecated breaking changes in e.g. the function signature of the moveBlockToPosition action. It's not expected this would be a very high-use function by third party plugins.
We no longer meaningfully use the attributes argument of insertDefaultBlock action, which makes for awkward ergonomics around insertDefaultBlock( undefined, rootClientId, index ). I might suggest we deprecate this first argument, though I have not done so here.
- For advanced use-cases "initial attributes" could still be supported anyways by insertBlock( createBlock( getDefaultBlockName(), initialAttributes ), rootClientId, index )

youknowriad · 2018-10-29T14:06:04Z

packages/editor/src/components/block-list/block.js

@@ -655,8 +650,7 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps ) => {
 			selectBlock( clientId, initialPosition );
 		},
 		onInsertBlocks( blocks, index ) {
-			const { rootClientId, layout } = ownProps;
-			blocks = blocks.map( ( block ) => cloneBlock( block, { layout } ) );


cloneBlock does more than just adding the layout, just want to ensure we don't break anything here. (For example, it clones the inner blocks too etc...)

I'm fairly certain this was only used to inject the layout, but I'll double-check.

The only thing which should change is the client ID, which is not something which should be relied upon anyways, and certainly not so for a block which is created but yet to be inserted.

youknowriad · 2018-10-29T14:06:22Z

packages/editor/src/components/block-list/block.js

@@ -670,10 +664,6 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps ) => {
 			mergeBlocks( ...args );
 		},
 		onReplace( blocks ) {
-			const { layout } = ownProps;
-			blocks = castArray( blocks ).map( ( block ) => (
-				cloneBlock( block, { layout } )


Same question as above.

youknowriad

I love it ❤️

youknowriad

Works great in my testing.

aduth · 2018-10-29T14:51:49Z

The Columns migration was indeed broken. It was a bit trickier to restore than I anticipated, since the layout was applied on its inner blocks, not the Columns itself. Should be fixed up in 45cb517, including tests for the migration.

aduth · 2018-10-29T14:53:27Z

test/integration/full-content/fixtures/core__columns__deprecated.serialized.html

@@ -0,0 +1,23 @@
+<!-- wp:columns {"columns":3} -->
+<div class="wp-block-columns has-3-columns"><!-- wp:column -->
+<div class="wp-block-column"><!-- wp:paragraph {"className":"layout-column-1"} -->


The className being kept after the migration may be unexpected, though conversely could better respect backward compatibility. I don't think it's a huge issue to keep, and also would be difficult to omit since the inner blocks are parsed before their parent, so a migration would otherwise need to re-parse and override.

chrisvanpatten · 2018-10-29T15:28:22Z

Pouring one out today for layouts.

Nice stuff; love the simplification.

aduth · 2020-05-26T19:12:16Z

The attributes merging of layout should likely have been removed here as well:

gutenberg/lib/class-wp-block-type.php

Lines 177 to 197 in 45cb517

    
           	/** 
        
           	 * Get all available block attributes including possible layout attribute from Columns block. 
        
           	 * 
        
           	 * @return array Array of attributes. 
        
           	 */ 
        
           	public function get_attributes() { 
        
           		return is_array( $this->attributes ) ? 
        
           			array_merge( 
        
           				$this->attributes, 
        
           				array( 
        
           					'layout' => array( 
        
           						'type' => 'string', 
        
           					), 
        
           				) 
        
           			) : 
        
           			array( 
        
           				'layout' => array( 
        
           					'type' => 'string', 
        
           				), 
        
           			); 
        
           	}

It still exists today, now in core.

See: https://core.trac.wordpress.org/ticket/50257

aduth added [Feature] Block API API that allows to express the block paradigm. [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P labels Oct 24, 2018

aduth added this to the API Freeze milestone Oct 24, 2018

aduth requested a review from chrisvanpatten October 24, 2018 21:23

chrisvanpatten reviewed Oct 25, 2018

View reviewed changes

aduth force-pushed the remove/layout branch 2 times, most recently from a2a0cd4 to 6cc32e9 Compare October 26, 2018 15:04

youknowriad modified the milestones: API Freeze, 4.2 Oct 27, 2018

aduth added 3 commits October 29, 2018 09:38

Editor: Deprecate layout attribute

c7d28be

Revert "Editor: Deprecate layout attribute"

007540f

This reverts commit c7d28be.

Editor: Remove non-functional layout support

4b17713

aduth force-pushed the remove/layout branch from 6cc32e9 to 4b17713 Compare October 29, 2018 13:57

youknowriad reviewed Oct 29, 2018

View reviewed changes

youknowriad approved these changes Oct 29, 2018

View reviewed changes

Blocks: Restore and test Columns migration

45cb517

aduth commented Oct 29, 2018

View reviewed changes

aduth merged commit 8db2764 into master Oct 29, 2018

aduth deleted the remove/layout branch October 29, 2018 15:18

aduth mentioned this pull request May 23, 2019

InnerBlocks layouts - no controls show #9714

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Deprecate `layout` attribute #11029

Editor: Deprecate `layout` attribute #11029

aduth commented Oct 24, 2018

gziolo commented Oct 25, 2018 •

edited

Loading

chrisvanpatten commented Oct 25, 2018 •

edited

Loading

aduth commented Oct 25, 2018

chrisvanpatten left a comment •

edited

Loading

youknowriad commented Oct 29, 2018

aduth commented Oct 29, 2018

aduth commented Oct 29, 2018

youknowriad Oct 29, 2018

aduth Oct 29, 2018

aduth Oct 29, 2018

youknowriad Oct 29, 2018

youknowriad left a comment

youknowriad left a comment

aduth commented Oct 29, 2018

aduth Oct 29, 2018

chrisvanpatten commented Oct 29, 2018

aduth commented May 26, 2020

Editor: Deprecate layout attribute #11029

Editor: Deprecate layout attribute #11029

Conversation

aduth commented Oct 24, 2018

gziolo commented Oct 25, 2018 • edited Loading

chrisvanpatten commented Oct 25, 2018 • edited Loading

aduth commented Oct 25, 2018

chrisvanpatten left a comment • edited Loading

Choose a reason for hiding this comment

youknowriad commented Oct 29, 2018

aduth commented Oct 29, 2018

aduth commented Oct 29, 2018

youknowriad Oct 29, 2018

Choose a reason for hiding this comment

aduth Oct 29, 2018

Choose a reason for hiding this comment

aduth Oct 29, 2018

Choose a reason for hiding this comment

youknowriad Oct 29, 2018

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

youknowriad left a comment

Choose a reason for hiding this comment

aduth commented Oct 29, 2018

aduth Oct 29, 2018

Choose a reason for hiding this comment

chrisvanpatten commented Oct 29, 2018

aduth commented May 26, 2020

Editor: Deprecate `layout` attribute #11029

Editor: Deprecate `layout` attribute #11029

gziolo commented Oct 25, 2018 •

edited

Loading

chrisvanpatten commented Oct 25, 2018 •

edited

Loading

chrisvanpatten left a comment •

edited

Loading