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

Blocks: Support block nesting (Add Columns block, InnerBlocks component) #3745

Merged
merged 13 commits into from
Feb 6, 2018

Conversation

aduth
Copy link
Member

@aduth aduth commented Nov 30, 2017

Closes #2995
Closes #428
Partially implements #219
Cherry-picks 1efbb9f and 2bd32f8 from #3743
Related: #3705, #2743
Partially reverts #2806 (temporarily)

This pull request seeks to explore support for generalized nested blocks, implementing a InnerBlocks component which effectively behaves as a sub-editor block listing (maintaining its own separate editor state). A block's children are available via its block.innerBlocks property, and can be accessed from a component's edit and save implementations (props.innerBlocks), and updated via edit's setInnerBlocks prop.

Included is a new Columns block which behaves similarly to the Text Columns block, except that it allows insertion of blocks within the rendered columns.

Nesting

What has been implemented:

  • Block parsing, creation, validation, and serialization of nested content
  • Editing block children, including insertion, rearrangement, toolbar and inspector controls

What doesn't work so well yet:

  • Front-end display
  • Reconciliation of selected blocks, particularly in rendering of toolbar and inspector slot contents
  • Reducing columns trims content instead of collapsing
  • General polish and UX behaviors
  • Tests and such

Testing instructions:

Verify that you can insert a new Columns block, and within it, insert, rearrange, edit, and delete child blocks. Verify that saving the post and refreshing the editor respects all changes which were made.

@aduth aduth added [Feature] Blocks Overall functionality of blocks Framework Issues related to broader framework topics, especially as it relates to javascript labels Nov 30, 2017
@georgestephanis
Copy link
Contributor

georgestephanis commented Dec 3, 2017

Summing up a brief discussion I had in person with @aduth , general wish-list and considerations (in no way is any of this a blocker or need-to-have for initial merge, just musings I wanted to get enumerated somewhere)

Another example (apart from layout use-cases) would be Contact Forms. Some bits for the future to support that use case:

  • Some blocks would not make sense in a non-nested context -- for example, Fields would not make sense unless you're adding them to a form. So registering blocks, but flagging them as disabled unless in a certain parent? Or adding them to a blacklist on the general context, to still allow them elsewhere? Unsure on implementation.
  • It would be useful to be able to define a default template for nested blocks (as per Framework: Bootstrap Block Templates #3668) -- so for example, a contact form instantiation could start with Name, Email Address, and Message fields every time. (or a Recipe block could start with an Image, List of Ingredients, and Directions blocks)

There were other things not immediately coming to mind, I'll add them as they come up.

@aduth
Copy link
Member Author

aduth commented Dec 4, 2017

Some blocks would not make sense in a non-nested context -- for example, Fields would not make sense unless you're adding them to a form. So registering blocks, but flagging them as disabled unless in a certain parent? Or adding them to a blacklist on the general context, to still allow them elsewhere? Unsure on implementation.

Noting that it wasn't specifically needed for a columns block (which should allow any child type), but I had an in-progress change to be able to support this via a block.parents array:

diff --git a/editor/components/inserter/menu.js b/editor/components/inserter/menu.js
index 73bfb7c0..4dc344fb 100644
--- a/editor/components/inserter/menu.js
+++ b/editor/components/inserter/menu.js
@@ -126,6 +126,12 @@ export class InserterMenu extends Component {
                                return false;
                        }
 
+                       // If block defines parent nesting, disallow except when explicitly
+                       // provided as a whitelisted block type.
+                       if ( Array.isArray( block.parents ) ) {
+                               return includes( blockTypes, block.name );
+                       }
+
                        // Block types defined as either `true` or array:
                        //  - True: Allow
                        //  - Array: Check block name within whitelist
@@ -311,7 +317,7 @@ export class InserterMenu extends Component {
                                }
                                { isSearching &&
                                        <div role="menu" className="editor-inserter__search-results">
-                                               { this.renderCategories( this.getVisibleBlocksByCategory( getBlockTypes() ) ) }
+                                               { this.renderCategories( this.getVisibleBlocksByCategory( this.getBlockTypes() ) ) }
                                        </div>
                                }
                        </TabbableContainer>

The thinking being a block could define it's allowed parents for nesting context, and conversely, a parent would define supportable children via a prop on BlockChildren (types={ [ 'core/text', ... ] }).

@aduth
Copy link
Member Author

aduth commented Dec 4, 2017

It would be useful to be able to define a default template for nested blocks (as per #3668) -- so for example, a contact form instantiation could start with Name, Email Address, and Message fields every time. (or a Recipe block could start with an Image, List of Ingredients, and Directions blocks)

I think this would be a good idea: Perhaps as an initialValue or defaultValue on BlockChildren, or as part of the block API to define default children. There's also a general question as to whether children needs to be a separate value of a block, or can be an attribute as any other (in which case leveraging attributes default behavior).

@hedgefield hedgefield mentioned this pull request Dec 5, 2017
2 tasks
attributes = attributes || {};

// Trim content to avoid creation of intermediary freeform segments
innerHTML = innerHTML.trim();
Copy link
Member

Choose a reason for hiding this comment

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

How was it being created otherwise?

Copy link
Member

Choose a reason for hiding this comment

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

I see it was already present just in a different moment.

* @return {string} Save content
*/
export function getSaveContent( blockType, attributes ) {
export function getSaveContent( blockType, attributes, children = [] ) {
Copy link
Member

Choose a reason for hiding this comment

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

What's the general thinking behind sometimes referring to innerBlocks and sometimes referring to children?

Copy link
Member Author

Choose a reason for hiding this comment

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

What's the general thinking behind sometimes referring to innerBlocks and sometimes referring to children?

We should probably align it. innerBlocks is the naming provided from the parser, but children seemed a friendlier / more familiar name. Maybe too familiar though, since it conflicts with the React built-in prop of the same name; could mislead a developer to think they could render it as an element set (which it is not).

Do you have a preference one way or the other?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd lean toward innerBlocks for the reasons you cite, plus the fact that with innerBlocks we reinforce the idea of "blocks" as the central object. Also, the notion of containment behind inner may be more fitting than that of descent behind children.

let content = renderToString( contentWithExtraProps );

// Drop raw HTML wrappers (support dangerous inner HTML without wrapper)
content = content.replace( /<\/?wp-raw-html>/g, '' );
Copy link
Member

Choose a reason for hiding this comment

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

Is it better to drop after rendering to string? What can be included in wp-raw-html?

Copy link
Member Author

Choose a reason for hiding this comment

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

Is it better to drop after rendering to string? What can be included in wp-raw-html?

This is a hacky workaround to the lack of <React.Fragment dangerouslySetInnerHTML /> support in React. It achieves the desired result by rendering a wrapper with a known name which is intended to be dropped during serialization. It would be used in cases where we want to render raw markup without a wrapper element (e.g. here for nested blocks, in #3048 for the saved output of Editable).

georgestephanis pushed a commit to Automattic/jetpack that referenced this pull request Dec 16, 2017
At this point it just represents the parent form -- it doesn't handle any fields yet.

That will largely depend on how WordPress/gutenberg#3745 progresses.
@aduth
Copy link
Member Author

aduth commented Jan 9, 2018

In rebasing this, I've found that our recent changes to treat the editor as a singleton store (#3832) conflict with the intent here to introduce nested block editing via separate store instances. @youknowriad do you have any suggestions on how I might proceed here? Maybe I just need to createStore with a direct reference to the reducer? I'll plan to spend some more time on this tomorrow.

@youknowriad
Copy link
Contributor

@youknowriad do you have any suggestions on how I might proceed here? Maybe I just need to createStore with a direct reference to the reducer?

Yes, I think using "Provider" and "it just works" was hiding the fact that the "blocks" become "local state" with the changes introduce in the current PR.

I think we should have a separate store for blocks (not a singleton) instantiated in the BlockChildren component. Ideally it would be used in the root level as well. and the main "store" would only contain a "blocks" reducer maybe being updated with the onChange of the BlockChildren component.

I've previously explored the idea in #2065

@aduth
Copy link
Member Author

aduth commented Jan 9, 2018

I think we should have a separate store for blocks (not a singleton) instantiated in the BlockChildren component. Ideally it would be used in the root level as well. and the main "store" would only contain a "blocks" reducer maybe being updated with the onChange of the BlockChildren component.

What do you have in mind being managed in each of these cases? A primary benefit of creating the nested provider context is not needing to consider implications of hierarchy and nesting in the store itself, which is quite convenient. It's also not just blocks, but state such as selection, hover, etc.

@youknowriad
Copy link
Contributor

@aduth

So the root store would handle "post", "layout" (since edit-post is still there), "saving", "metaboxes", "editor", "edits"...

The blocks store would be "blocks", "selection", "hover" (all what's block related)

Do we need some of these informations in the upper level? maybe, I don't know right now, this is discoverable while implementing.


This is not different from what you were doing originally. It's just making it clear that the "BlocksProvider" is about the blocks only since I assume the other parts of the store were useless in the "BlockChildren", am I wrong?

I believe a quicker alternative to mimic exactly what you were doing is to recreate the store, expose a createStore from "editor/store/index.js" and use it in BlockChildren. Maybe I'm wrong but I think the separate stores is nicer, it clarifies what handles what.

@jorgefilipecosta
Copy link
Member

Hi @aduth, I have been testing this branch and generally, it works well, nice work 👍 There are some things to polish but I think this is a good shape and we may solve polish/solve some bugs.

Regarding the singleton store vs multiple stores discussion, while the multiple stores being used may make things simple at the start, I think it may cause us some problems in the future to make some features works as expected. E.g: undo may affect the content of multiple stores.

I think we may be able to use a single store. We can have a higher order reducer "H" that would apply to a reducer that has "multiple instances" "S" e.g: editor, blockSelection. We would dispatch an action to create an instance e.g:"{type: CREATE_EDITOR_CONTEXT, id: uuid}. The reducer "H" would create the instance for us e.g: assign to an object the reducer "S" passed to it to generate the empty state.
The actions affecting the reducer "S" would need to contain the uuid and our higher order reducer "H" would call the reducer with the right state branch. If no uuid is passed our higher order reducer "H" calls our reducer "S" on all branches.
The selectors selecting branches "S" would receive only the branch instead of the state, there would exist a selector to select the right branch based on the uuid.
With this approach, we would be able to apply actions to all the "contexts" e.g: remove all multi-selections, and we would retain the advantages of a single store, e.g: being able to debug with redux dev tools.

@youknowriad
Copy link
Contributor

Just a small note that the separate store approach (which is just a convenient way to build a complex local state for a react component) brings us closer to the "Block Editor" being a generic JS module reusable in several contexts (inside WordPress aka Customizer for instance and outside Wordpress).

@mcsf
Copy link
Contributor

mcsf commented Jan 9, 2018

With regards to nesting proper—ignoring the weaknesses cited in the description—this seems to work well.

BlockChildren is quite interesting. I like the idea that we can swap contexts at that membrane and let its element subtree behave without necessary knowledge of its own nesting. A priori this seems more viable than juggling enhanced reducers, although I used to feel more strongly about striving for a single store.

@aduth aduth changed the title Blocks: Support block nesting (Add Columns block, BlockChildren component) Blocks: Support block nesting (Add Columns block, InnerBlocks component) Jan 9, 2018
@aduth
Copy link
Member Author

aduth commented Jan 9, 2018

Force pushed a rebased copy of the branch, bringing the changes up to date, reconciling with the revised state approach, and being more consistent with the naming of "inner blocks" vs. "children" (deferring to the former, since the latter is overloaded already).

I'm observing some strange behaviors with toolbar interactions, which might be related to the block selection not working correctly anymore in the nested block (should be deselecting parent). Going to see if recent revisions in #3743 improve this.

Needs a plan for front-end styling, and for tests to be updated.

@aduth
Copy link
Member Author

aduth commented Jan 9, 2018

Noting another issue is that trying to "Show Advanced Settings" from a block in the visual editor will not display the sidebar. This is because since the nested block component maintains its own full copy of the editor store, it's also expected to manage the display of sidebars (which it doesn't). Toward @youknowriad's earlier points, we likely need to break this down further where the only state managed by InnerBlocks is that of the blocks it's showing.

@youknowriad
Copy link
Contributor

Some notes:

  • Is it intentional to make the columns block "wide" or just a leftover?
  • The inspector shows only the container options, how do we show the inner block inspector
  • The columns block doesn't have a block toolbar, what behavior should it have if it had one (I assume showing the inner block's toolbar unless the "container" is selected)
  • I tried multi-selecting some children blocks and clicking backspace. It doesn't work

If we want to make this perfect before merging, it will take a lot of time. So we might want to make a decision here. Do we want to merge this (even a bit broken sometimes) and iterate on it (which could lead to a lot of issues created once released), or should we iterate on the PR which could lead to a giant PR.

@mtias
Copy link
Member

mtias commented Jan 10, 2018

I am fine with merging an initial implementation if we mark the "columns block" with some text that says it is experimental, and provided we don't break other areas of the editor.

@JasonHoffmann
Copy link
Contributor

Not sure if this is exactly the right place for this, but I wanted to hop in and just highlight another use case I've been running into. Being able to embed / nest one block inside of another programmatically in a more structured way.

For instance, let's say I have a grid of items. Each one has an image, a title, and then some "paragraph" text. What I want to be able to do is register a custom block that allows users to select how many columns in this grid they want to add, and then for each one, nest an "image" a "heading" and a "paragraph" block inside of a larger column container. Right now (I think) I'd have to recreate the functionality for each custom block, but it would be great just to do something like addEditInnerBlock( 'core/paragraph' );

I'm not sure if this means creating an InnerBlock component that takes the name of a block as a property, or if there's even a way to do it now. But I wanted to flag it because I think being able to make pre-structured, nested blocks will be a pretty common issue for developers.

@carlhancock
Copy link

Why the insistence on defaulting the columns to having Paragraph blocks in them instead of simply having an insert block UI within the column as the default?

By defaulting to paragraph blocks already being in place it means people are constantly going to be deleting the default block after placing the actual blocks they want to use in the columns.

To make it even more annoying... if they don't do things in the correct order when doing what i've described above the process is going to be frustrating. For example if I add a Columns block to my page and then I edit a column and say I don't want a paragraph here I want an image... so I delete the Paragraph block that is there by default. It then adds another default Paragraph block. You have to add the block you actually want to use below that paragraph block and then delete the paragraph block.

I think the assumption that everything defaults to a Paragraph is a mistake. There are going to be all kinds of blocks created by Gutenberg and the insistence on defaulting to Paragraph is a very blog-centric way of looking at things. It would be a lot more helpful to look at things from a WordPress as a CMS standpoint.

It would be a much better user experience if columns allowed you to start from scratch and didn't make the assumption that the first block should be a Paragraph and instead simply provided an insert block UI so you could insert only the blocks you want to insert and not have to go back and clean up what Gutenberg has done for you by default.

@gubbigubbi
Copy link

Great work team.

New (experimental) Columns block
Note: This block is implemented using CSS grid styling (browser support)

CSS grid is the future, but should we look to use flexbox for now, it has much wider browser support? prefixed Flexbox works as far back as IE10.

Eventually we will also need the following features:

  • Mobile / tablet / desktop columns
  • Padding and margins for each column

@aduth
Copy link
Member Author

aduth commented Feb 7, 2018

Why the insistence on defaulting the columns to having Paragraph blocks in them instead of simply having an insert block UI within the column as the default?

Where was this insisted? The current behavior of the Columns block is implemented as it is largely as a consequence of it inheriting behaviors of the underlying BlockList components it reuses. In other words, the block is something of an embedded editor. As an initial exploration, the UX of the block is very unfinished, and I would expect there to be some iterations more toward what you're describing with optimizing layout editing.


Because `InnerBlocks.Content` will generate a single continuous flow of block markup for nested content, it may be advisable to use [CSS Grid Layout](https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_Grid_Layout) to assign layout positions. Be aware that CSS grid is [not suported in legacy browsers](https://caniuse.com/#feat=css-grid), and therefore you should consider how this might impact your block's appearance when viewed on the front end of a site in such browsers.

Layouts can be assigned either either as an object (ungrouped layouts) or an array of objects (grouped layouts). These are documented below.
Copy link
Contributor

@jaswrks jaswrks Feb 7, 2018

Choose a reason for hiding this comment

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

Layouts can be assigned either eithereither.
Oh, already merged. So nevermind, I will fix in a PR. #4910

@jasmussen
Copy link
Contributor

Why the insistence on defaulting the columns to having Paragraph blocks in them instead of simply having an insert block UI within the column as the default?

I responded in more detail to a similar question you asked here.

We already have insertion UI in the editor bar, and on the side, and using the slash command for the power users. These all work in a nested context, but require your caret to be where you want the content to be inserted, similar to most other editors that let you insert non-text content, like Word or Pages or Google Docs.

When #3627 gets addressed, the concern of "why is there already a paragraph block here" becomes moot, as any content you insert while the caret is an empty paragraph block replaces it.

lsl pushed a commit to lsl/jetpack that referenced this pull request Apr 6, 2018
At this point it just represents the parent form -- it doesn't handle any fields yet.

That will largely depend on how WordPress/gutenberg#3745 progresses.
lsl pushed a commit to lsl/jetpack that referenced this pull request Apr 23, 2018
At this point it just represents the parent form -- it doesn't handle any fields yet.

That will largely depend on how WordPress/gutenberg#3745 progresses.
lsl pushed a commit to lsl/jetpack that referenced this pull request May 1, 2018
At this point it just represents the parent form -- it doesn't handle any fields yet.

That will largely depend on how WordPress/gutenberg#3745 progresses.
@yaplusplus
Copy link

Hey, thanks for the great work! Are there any plans to implement multiple InnerBlocks instances inside a nested block? We're making our own WYSIWYG editor and we've developed a special wrapper block to insert the editor into a Gutenberg post. Now we're thinking how to make our editor contain another nested Gutenberg blocks inside its content (note: our editor uses it's own content structure that differs from Gutenberg's).

We use code like that for running the editor:

import SetkaEditor from './SetkaEditor';

export default class SetkaEditorBlock extends Component {

    componentDidMount() {
        // Run editor at the specified DOM node
        SetkaEditor.start(this._domElement, this.props.attributes.content);
    }

    render() {
        // Render wrapper block
        return (
            <div>
                <div className="stk-editor" ref={el => this._domElement = el} />
            </div>
        )
    }

}

And probably the final HTML code containing both our and Gutenberg blocks should look like this:

<!-- wp:setka-editor/setka-editor -->

<div class="stk-post">

  <p>Our editor's text paragraph</p>

  <!-- wp:block-1 -->
  <p>Gutenberg block</p>
  <!-- /wp:block-1 -->

  <div>
    <h2>Our editor's heading</h2>

    <!-- wp:block-2 -->
    <div>Another Gutenberg block<div>
    <!-- /wp:block-2 -->
  </div>

</div>

<!-- /wp:setka-editor/setka-editor -->

One of the problems is that we don't know where to place <InnerBlocks /> component because Gutenberg blocks could be possibly placed anywhere inside our editor's content multiple times.

So we have two questions:

  1. Is such kind of integration possible in general?
  2. If yes, which Gutenberg API should help us to implement it?

Any thoughts and advices, pros and cons will be highly appreciated.

lsl pushed a commit to Automattic/jetpack that referenced this pull request Jul 12, 2018
At this point it just represents the parent form -- it doesn't handle any fields yet.

That will largely depend on how WordPress/gutenberg#3745 progresses.
@kmukku
Copy link

kmukku commented Aug 30, 2018

Hi, first of all I'm impressed of the whole Gutenberg project and efforts you guys have made to improve the editing experience in Wordpress. Good job!

I have been lately working mostly with other CMS, which has had this sort of rendering engine for a long time. In previous versions it used to have some sort of column block like you have here, for nesting blocks, but it was removed as it was not necessary after all. I made a demo some time a go to demonstrate how to use CSS frameworks (same applies to CSS Grid) in order to manage the block rendering in UI. You can see it here:

http://jsfiddle.net/kmukku/zz83dx0a/

Nothing fancy, but it gives the idea how to avoid use of column blocks. I'm sure this could be implemented pretty easily into Gutenberg too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks Framework Issues related to broader framework topics, especially as it relates to javascript
Projects
None yet
Development

Successfully merging this pull request may close these issues.