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.

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

Limit of Next Page (Page Break) block to root level only #18260

Merged

Conversation

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Nov 4, 2019

Description

Adds a new 'root' parent block type which allows a block to be limited to only insertion in the root context. This is initially as a suggested fix for a bug with the Page Break block ( #16411 ) but could be documented as a way to limit any block to the root context.

How has this been tested

Just tested manually at this stage. If it is agreed that this is a possible way forward I will look at adding some unit or e2e tests.

To test

  • Check out this branch
  • Add a Page Break block to the editor root - should work as normal
  • Try adding a Page Break block to a block that allows nesting like Columns - it should not appear as an available block to add

Screenshots

pagebreak

… to root level only
@glendaviesnz glendaviesnz changed the title WIP - Limit of next page block to root level only Limit of Next Page (Page Break) block to root level only Nov 11, 2019
@glendaviesnz glendaviesnz marked this pull request as ready for review Nov 11, 2019
@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Nov 11, 2019

This seems to work, and doesn't break any existing tests, but I am sure I am overlooking some gotchas, or an easier existing way to limit a block to the root context.

@mmtr

This comment has been minimized.

Copy link
Contributor

mmtr commented Nov 11, 2019

This seems to work, and doesn't break any existing tests, but I am sure I am overlooking some gotchas, or an easier existing way to limit a block to the root context.

I think this is a valid approach! Simple and easy 🙂

@kwight

This comment has been minimized.

Copy link

kwight commented Nov 11, 2019

Works really well 👍

@@ -1036,6 +1036,9 @@ const canInsertBlockTypeUnmemoized = ( state, blockName, rootClientId = null ) =
return list;
}
if ( isArray( list ) ) {
if ( includes( list, 'root' ) && item === null ) {

This comment has been minimized.

Copy link
@ellatrix

ellatrix Nov 28, 2019

Member

How would it work if it's not a post, in case of full site editing? Maybe not something we should fix now, but maybe good to have in mind. Cc @epiqueras.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Nov 28, 2019

Contributor

It would only let you insert it at the root level of the template.

I guess we would need to add core/post-content to the list of allowed parents? Along with any other template layout blocks that should be able to insert a page break.

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 2, 2019

Author Contributor

Could we also add a list of parents that should be considered as a 'root' container at this point, rather than having to add them to each blocks allowed parents? eg.

const rootContainers = [ null, 'core/post-content ];
if ( includes( list, 'root' ) && includes( rootContainers , item )  )

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 2, 2019

Contributor

Sure, but won't the page break still break if its rendered in a Post Content block? Or does it only break in Column blocks?

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 2, 2019

Author Contributor

I just used Post Content block as an example because you had mentioned it - the array would just contain any parent blocks that we knew should be treated like a root element for the likes of page breaks.

It may be that it is not possible to identify what constitutes a 'root' block for a range of different child blocks, in which case it is probably best to leave 'root' purely as the editor root with if ( includes( list, 'root' ) && item === null ), and add any other possible parent blocks to the blocks own allowed parents array.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 2, 2019

Contributor

It may be that it is not possible to identify what constitutes a 'root' block for a range of different child blocks, in which case it is probably best to leave 'root' purely as the editor root with if ( includes( list, 'root' ) && item === null ), and add any other possible parent blocks to the blocks own allowed parents array.

A "root" block shouldn't be defined in relationship to a range of child blocks. A "root" block is simply a block that serves as the root of the block tree in some rendering context. For example, a Post Content block serves as the root of the block tree when rendering a single post, but doesn't when rendering an entire template. So I think we are safe with including Post Content in our definition of "root".

I just used Post Content block as an example because you had mentioned it - the array would just contain any parent blocks that we knew should be treated like a root element for the likes of page breaks.

Yeah, I understood that. I was just wondering if the Page Break block will cause issues as a child of the Post Content block. I think it should be fine.

Now that I think more about this. The Page Break block only makes sense as a top level block in post content. If we whitelist this using "root", you would be able to add them to templates and template parts which wouldn't make any sense or work.

To make this work, the selector needs to check that the block is being inserted at the root level and that we are editing a post, or that it's being inserted into a Post Content block.

that we are editing a post

We don't have a canonical way of doing this yet so I think that the way to move forward is using

parent: [ 'core/post-content' ]

and

if ( includes( list, 'core/post-content' ) && item === null ) {

for now. With a todo note to change it to

if (
  includes( list, 'core/post-content' ) &&
  (
    item === 'core/post-content' ||
    (getEditorMode() === 'post-content' && item === null)
  ) {

when possible.

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 3, 2019

Author Contributor

That approach makes sense to me - have made this change, but haven't had time to test it properly - will do that tomorrow hopefully.

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 3, 2019

Contributor

Looks good to me.

We'll actually only need to change it to includes( list, 'core/post-content' ) && getEditorMode() === 'post-content' && item === null, because the following return includes( list, item ); will take care of the other case where item === 'core/post-content'.

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 4, 2019

Author Contributor

Have updated the TODO comment. Will try and get a unit test added in the next day or so.

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 9, 2019

Author Contributor

@epiqueras, finally got back to this, have added a basic unit test to cover this.

Glen Davies
Glen Davies added 2 commits Dec 4, 2019
Glen Davies
Glen Davies
packages/block-editor/src/store/selectors.js Outdated Show resolved Hide resolved
packages/block-editor/src/store/test/selectors.js Outdated Show resolved Hide resolved
packages/block-editor/src/store/test/selectors.js Outdated Show resolved Hide resolved
blocks: {
byClientId: {},
attributes: {
block1: {},

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 9, 2019

Contributor

This is unnecessary.

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 9, 2019

Author Contributor

fixed

@@ -121,6 +121,15 @@ describe( 'selectors', () => {
},
} );

registerBlockType( 'core/post-content-child', {
save: ( props ) => props.attributes.text,

This comment has been minimized.

Copy link
@epiqueras

epiqueras Dec 9, 2019

Contributor

This block doesn't have attributes.

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 9, 2019

Author Contributor

fixed

glendaviesnz and others added 5 commits Dec 9, 2019
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Co-Authored-By: Enrique Piqueras <epiqueras@users.noreply.github.com>
Glen Davies
@glendaviesnz

This comment has been minimized.

Copy link
Contributor Author

glendaviesnz commented Dec 9, 2019

@epiqueras - apologies for the basic formatting issues, my local linting doesn't seem to be working and hadn't noticed.

@glendaviesnz glendaviesnz requested a review from epiqueras Dec 9, 2019
Copy link
Contributor

epiqueras left a comment

Thanks!

@epiqueras epiqueras merged commit 2ebfb64 into WordPress:master Dec 9, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@glendaviesnz glendaviesnz deleted the glendaviesnz:fix/page-break-as-child-bug branch Dec 9, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.