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

Editor: Implement the EntityHandlers component and basic template blocks. #17020

Conversation

epiqueras
Copy link
Contributor

Description

The EntityHandlers component is an algebraic effects (https://overreacted.io/algebraic-effects-for-the-rest-of-us) inspired API, for tackling full site editing and blocks that sync to different posts and/or third party APIs, that I've been discussing with a few people.

For context:

Just like how React Suspense throws promises for the nearest handler to catch and execute loading logic, and how Redux Saga lets you yield the intent to perform an effect to a handler higher up in the call stack, a block edit in the block editor package could be interpreted as intent to perform a persistence effect. This viewpoint let us clean up the API design into something much more declarative and composable. A single EntityHandlers component that would receive an “entity” or “entityId” prop and a list of edits it would handle in a “handles” prop. These can be nested in a hierarchy and when an edit is made it would bubble up to the nearest component that handles it. They can also take an “independentSave” prop, that when true will render an inline UI for saving, kind of like reusable blocks today, and when false will simply trigger saving when its parent does, allowing the “save” action to also be composed in a declarative hierarchy when it makes sense for blocks to have different save flows. This means that blocks like the post content block can easily declare that they handle all edits, and blocks like the post block can handle most edits, but let content bubble up to the template. We then realized that if we added a client side “blocks” property to entities, it would be trivial to use inner blocks and keep dirty states isolated. This meant attributes in the block list that use custom sources to sync to other entity properties would be duplicated, so @youknowriad revisited our approach to custom sources and decided it would be better for the EntityHandlers components to just provide children with functions for reading from, and editing the relevant entity through context. This would make it a lot easier to make blocks that sync with dates, meta properties, and other properties outside of content.

Another great benefit of this approach is that it moves most of the WordPress specifics further away from the editor and into the entity abstraction. Very soon, all it will take to render an editor that persists to somewhere other than a post, will be to implement a new entity. The editor will be a fully composable abstraction of its own.

After working on #16932, I branched off of that and managed to implement this previously hypothetical API with very little lines of code and used it to implement a "Post" block for templates and full site editing.

The usage is very simple:

export default function PostEdit( { attributes: { postId } } ) {
	const entity = useSelect(
		( select ) =>
			select( 'core' ).getEntityRecord( 'postType', 'post', postId ),
		[ postId ]
	);

	return entity ? (
		<EntityHandlers entity={ entity } />
	) : (
		<Placeholder>
			<Spinner />
		</Placeholder>
	);
}

And if you want to save serialized content into the parent entity:

export default function PostSave() {
	return <EntityHandlers.Content />;
}

Implementing this as inner blocks that also sync to a nested editor store, in a way similar to how the block-editor store syncs to the editor store, turned out to be super intuitive and fell right into place.

(In Progress)

The new post block is currently syncing everything, but we want it to sync everything but post content. The idea is for the post block to save its content to a parent template entity, but support things like a nested post title and post content block that work as expected. Like this:

aww-board (1) (1)

EntityHandlers does not support this yet. But I will work on that next and I expect it to be pretty simple. I think I'll make each editor store keep track of its handles prop value and whenever a block change affects an entity property it will first check:

  1. That it handles that property.
  2. That no child registry has an editor store that handles it.

Because block changes "bubble up" through all nested registries up to the top level one, this should work well.

After this is well tested and merged, I'll work on simplifying the custom sources implementation as suggested in my quoted rambling above. That should lean out the editor store even more.

For the independentSave functionality, I plan to have the editor store traverse child registries, stopping at the ones where independentSave is set to true, and then trigger saving in the entire path it went through.

How has this been tested?

The new post block was used and it was verified that nesting posts worked as expected and all loaded posts saved correctly.

Screenshots

ezgif com-video-to-gif (3)

Types of Changes

New Feature: Add a new EntityHandlers component for creating blocks that sync to multiple posts or even third party APIs, and use it to create a "Post" block that nests a post.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@epiqueras epiqueras added [Feature] Blocks Overall functionality of blocks [Package] Blocks /packages/blocks [Package] Editor /packages/editor [Package] Block library /packages/block-library [Package] Block editor /packages/block-editor labels Aug 13, 2019
@epiqueras epiqueras added this to the Future milestone Aug 13, 2019
@@ -69,6 +83,31 @@ class InnerBlocks extends Component {
this.synchronizeBlocksWithTemplate();
}
}

// Sync with controlled blocks value from parent, if possible.
if ( this.isSyncingIncomingBlocks === innerBlocks ) {
Copy link
Member

Choose a reason for hiding this comment

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

I think this logic is not going to work because innerBlocks will never have the same reference that isSyncingIncomingBlocks.
We call:
this.isSyncingIncomingBlocks = blocks;
replaceInnerBlocks( blocks );

When the replace operation is finished and we get new innerBlocks the reference of these inner blocks is not the same as the blocks we passed to replaceInnerBlocks. Because blocks are not saved as a tree in the state and the reference we get is new computation created by the getBlock selector.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When the replace operation is finished and we get new innerBlocks the reference of these inner blocks is not the same as the blocks we passed to replaceInnerBlocks.

That's not ideal, could we get around it somehow? Maybe the reducer can keep a reference to the original array or something. In any case, this logic should not cause any issues if we leave it in until an enhancement is made to the store.

…n a specific implementation or the "editor" module.
@epiqueras
Copy link
Contributor Author

@youknowriad

Thanks for reviewing this again.

Trying to review this. Again! I have a very different vision from what you propose here. I think it's simpler and also don't require any custom sources (the API I shared before) and it doesn't require any change to how EditorProvider or BlockEditorProvider works.

I feel like I might have to do it separately to compare.

This API does not have custom sources, it removes them, and provides direct access to get and set entity attributes. The API is virtually identical to your proposal (useEntityProperty === useEditedEntityAttribute) except for the fact that it sets up this syncing mechanism between inner blocks and a new editor store. I think this is the best way to sync blocks and content in sub entities. How else would you do it?

Also, I think we should work on this PR with a concrete block (site title block) (not post title because it's a different type of entity and could create more challenges)

Good idea, that's high up on my list and I can work on it next. I don't think it will have any issues.

@jorgefilipecosta

Hi @epiqueras, thank you for moving this work forward! I like the general concept of how an Entity handler allows changing the place where blocks in an InnerBlocks area are saved 👍

Thanks!

Everything you described is the expected behavior. You are thinking of a post content block which would go inside a post block. The sketch in the description of this PR and the quoted text should make this clearer. The demo I posted here: #17020 (comment), also explains it a bit.

@youknowriad
Copy link
Contributor

I think this is the best way to sync blocks and content in sub entities. How else would you do it?

Good observations, that's the part I don't like the most because I feel it's just adding complexity but not worth the effort. I'm not entirely certain we won't need it in the end but I feel we can start without it pretty easily by just something very similar to this in the PostContent block.

https://github.com/WordPress/gutenberg/pull/14367/files#diff-ae0ba49328ef77ebd8fc08a86fee257aR120-R125

The only difference would be that the value and onChange handler would come and rely on useEntityProperty like any other entity property (it would use the blocks entity property).

@epiqueras epiqueras force-pushed the update/the-editor-store-to-use-core-data-entities branch from d596db7 to 1f1cdb6 Compare August 15, 2019 21:32
@epiqueras
Copy link
Contributor Author

Most of that PR could be implemented in a much simpler way using EntityHandlers. That approach creates a new block editor provider at every level? So it loses the benefit of having a single block list. It also looks way more complex and tied to reusable blocks. A reusable block should just be a block that uses EntityHandlers, like the Post block in this PR.

@epiqueras
Copy link
Contributor Author

The Site Title Block:

/**
 * WordPress dependencies
 */
import {
	useEditEntity,
	RichText,
	useEditedEntityAttribute,
} from '@wordpress/block-editor';
import { __ } from '@wordpress/i18n';
import { useSelect } from '@wordpress/data';
import { EntityHandlers } from '@wordpress/editor';
import { Placeholder, Spinner } from '@wordpress/components';

function TitleInput() {
	const editEntity = useEditEntity();
	return (
		<RichText
			value={ useEditedEntityAttribute( 'title' ) }
			onChange={ ( title ) =>
				editEntity( {
					title,
				} )
			}
			tagName="h1"
			placeholder={ __( 'Site Title' ) }
			formattingControls={ [] }
		/>
	);
}

export default function SiteTitleEdit() {
	const entity = useSelect(
		( select ) => select( 'core' ).getEntityRecord( 'root', 'site' ),
		[]
	);
	return entity ? (
		<EntityHandlers entity={ entity } handles={ { title: true } } noInnerBlocks>
			<TitleInput />
		</EntityHandlers>
	) : (
		<Placeholder>
			<Spinner />
		</Placeholder>
	);
}

@epiqueras
Copy link
Contributor Author

Now Post Content:

/**
 * WordPress dependencies
 */
import { EntityHandlers } from '@wordpress/editor';

export default function PostContentEdit() {
	return (
		<EntityHandlers handles={ { content: true, blocks: true } } />
	);
}

@epiqueras
Copy link
Contributor Author

The whole lineup:

Post Block:

export default function PostEdit( { attributes: { postId }, setAttributes } ) {
    const entity = useSelect(
        ( select ) =>
            postId && select( 'core' ).getEntityRecord( 'postType', 'post', postId ),
        [ postId ]
    );

    if ( ! postId ) {...}

    return entity ? (
        <EntityHandlers
            entity={ entity }
            // Let content and blocks bubble up to a parent template entity.
            handles={ { all: true, content: false, blocks: false } }
        />
    ) : (
        <Placeholder>
            <Spinner />
        </Placeholder>
    );
}

Post Title Block:

export default function PostTitleEdit() {
    const editEntity = useEditEntity();
    return (
        <RichText
            value={ useEditedEntityAttribute( 'title' ) }
            onChange={ ( value ) =>
                editEntity( {
                    title: value,
                    slug: cleanForSlug( value ),
                } )
            }
            tagName="h1"
            placeholder={ __( 'Title' ) }
            formattingControls={ [] }
        />
    );
}

Post Content Block (cc @jorgefilipecosta):

export default function PostContentEdit() {
    return (
        <EntityHandlers handles={ { content: true, blocks: true } } />
    );
}

Site Title Block:

export default function SiteTitleEdit() {
    const entity = useSelect(
        ( select ) => select( 'core' ).getEntityRecord( 'root', 'site' ),
        []
    );
    return entity ? (
        <EntityHandlers entity={ entity } handles={ { title: true } } noInnerBlocks>
            <TitleInput />
        </EntityHandlers>
    ) : (
        <Placeholder>
            <Spinner />
        </Placeholder>
    );
}

@youknowriad
Copy link
Contributor

In the example above why are we using EntityHandlers component in the SiteTitle block? I thought this component was only used for the "producer" not the "consumer"?

That approach creates a new block editor provider at every level?

yes, it creates a BlockEditorProvider at the level where you want to "split" the block list into two block lists. As we talked previously, I don't think there's any value in keeping a single block list containing all the blocks in that case because these are things that get stored separately.

When it comes to interaction like Drag & Drop..., we do need to handle cross block list drag and drop anyway, regardless of that change (think of widgets screen for instance).

It also looks way more complex and tied to reusable blocks. A reusable block should just be a block that uses EntityHandlers, like the Post block in this PR.

It's not tied to reusable blocks in any way, I just showed you an example of its usage in the reusable blocks PR. But the idea is just to replace your EntityHandlers.Content with BlockEditorProvider and useEditedEntityAttribute to get the value without any change to the framework level (there's no need handles prop for EntityHandlers, there's no need to hack the EditorProvider` component to have a way to ignore block changes...), it's way simpler for me. I don't see any complexity here.

@epiqueras
Copy link
Contributor Author

In the example above why are we using EntityHandlers component in the SiteTitle block? I thought this component was only used for the "producer" not the "consumer"?

#14367 (comment)

yes, it creates a BlockEditorProvider at the level where you want to "split" the block list into two block lists. As we talked previously, I don't think there's any value in keeping a single block list containing all the blocks in that case because these are things that get stored separately.

In #14367 it looks like it introduced quite a few issues. They are definitely fixable, but we get them for "free" with this approach.

It's not tied to reusable blocks in any way, I just showed you an example of its usage in the reusable blocks PR. But the idea is just to replace your EntityHandlers.Content with BlockEditorProvider and useEditedEntityAttribute to get the value without any change to the framework level (there's no need handles prop for EntityHandlers, there's no need to hack the EditorProvider` component to have a way to ignore block changes...), it's way simpler for me. I don't see any complexity here.

It is more complex for block developers. How would you implement this set of blocks:

#17020 (comment)

image

I don't think it's even possible with your approach, because there is no way to delegate certain edits to a parent entity.

@youknowriad
Copy link
Contributor

I don't think it's even possible with your approach, because there is no way to delegate certain edits to a parent entity.

That's actually super easy in my approach because you choose which edit you delete and which you don't. If you're thinking how the PostTitle block saves its children to the template, it just uses InnerBlocks like any other block.

@youknowriad
Copy link
Contributor

In the example above why are we using EntityHandlers component in the SiteTitle block? I thought this component was only used for the "producer" not the "consumer"?

#14367 (comment)

This doesn't answer my question. What If I have two sites and I want to "Site Block wrappers" and use the Site Title under each wrapper to edit the site of the two sites? How do we differentiate between which site I'm using?

Basically what I'm saying is that the Blocks that edit the properties shouldn't be the one that define the entity. Imagine a PostList block containing multiple Post blocks a SiteList block containing multiple Site blocks and inside each one of them a SiteTitle Block, a SiteDescription block...

@youknowriad
Copy link
Contributor

In #14367 it looks like it introduced quite a few issues. They are definitely fixable, but we get them for "free" with this approach.

I'm well aware and as I said, The only one that's related to the approach of using the BlockProvider is the Drag & Drop and as I said we need to fix cross block list drag and drop anyway (widgets screen) but even without it I don't think it's critical enough to introduce the complexity needed for this to work.

@youknowriad
Copy link
Contributor

As always it's better to share code to discuss. So here's approximatively what I propose.

https://gist.github.com/youknowriad/a061b46fc7da8de41e0d9cc2beeb27b4

@epiqueras
Copy link
Contributor Author

That's actually super easy in my approach because you choose which edit you delete and which you don't. If you're thinking how the PostTitle block saves its children to the template, it just uses InnerBlocks like any other block.

"Post Title" shouldn't have any children. How does "Post" save its children to the template, but at the same time set the context for where its child "Post Content" should save to?

This doesn't answer my question. What If I have two sites and I want to "Site Block wrappers" and use the Site Title under each wrapper to edit the site of the two sites? How do we differentiate between which site I'm using?

Your question was:

In the example above why are we using EntityHandlers component in the SiteTitle block? I thought this component was only used for the "producer" not the "consumer"?

That does answer your question. Certain blocks like "Site Title" might want to provide and consume the entity in a contained manner. Now, multi-site "Site Title" blocks are a new thing, but we could easily support them because each "Site Title" sets its own entity so each one would just set a different site entity.

Basically what I'm saying is that the Blocks that edit the properties shouldn't be the one that define the entity. Imagine a PostList block containing multiple Post blocks a SiteList block containing multiple Site blocks and inside each one of them a SiteTitle Block, a SiteDescription block...

Yes, that's how the "Post" and "Post Content" blocks work here. We could do the same for the "Site Title", but it doesn't make sense because we are in the context of a single site for now. If we need that we can just add that functionality in. That's not a flaw of this approach, it's just an implementation detail of "Site Title".

I'm well aware and as I said, The only one that's related to the approach of using the BlockProvider is the Drag & Drop and as I said we need to fix cross block list drag and drop anyway (widgets screen) but even without it I don't think it's critical enough to introduce the complexity needed for this to work.

It also breaks the inspector, multi-selections, and the global inserter from what I see.

https://gist.github.com/youknowriad/a061b46fc7da8de41e0d9cc2beeb27b4

That looks nice. But, it does not support multiple posts, custom entities, etc.

@youknowriad
Copy link
Contributor

"Post Title" shouldn't have any children. How does "Post" save its children to the template, but at the same time set the context for where its child "Post Content" should save to?

I guess that's answered by the gist, right? It just uses InnerBlocks and the EntityProvider just set the context of its innerBlocks.

That looks nice. But, it does not support multiple posts, custom entities, etc.

It does support multiple posts. A PostTitle used at any place in the hierarchy will always target one Post any way right? you can use the Post block as much as you want to define other React trees targetting different posts.

custom entities

We don't support custom entities in core-data anyway but if someone wants to build his own EntiityProvider that targets his own APIs, .... that should be easy to build anyway. And once we introduce custom entities support in core-data, I can see this being easily update to support those.

@epiqueras
Copy link
Contributor Author

Closed in favor of #17153.

@epiqueras epiqueras closed this Aug 27, 2019
@youknowriad youknowriad deleted the try/implementing-entity-handlers branch September 9, 2019 10:06
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 [Package] Block editor /packages/block-editor [Package] Block library /packages/block-library [Package] Blocks /packages/blocks [Package] Editor /packages/editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants