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

Block Library: Implement template blocks using nested block editors. #17118

Conversation

epiqueras
Copy link
Contributor

Description

This PR is an alternative approach to #17020 for providing entity context to blocks and implementing template blocks that sync to different entities.

Instead of syncing inner blocks to different entities like #17020, this PR leverages the work of @youknowriad's reusable block refactor (#14367), and uses nested block editors instead. This provides for a simpler implementation, albeit at the cost of breaking up the block list. This means we lose block navigation, simple drag and drop/moving, block inspectors, save flow/change detection UI, integrated undo UI, etc.

How has this been tested?

The blocks were tested manually to work as expected.

Types of Changes

New Feature: Implement new template blocks: Site Title, Post, Post Title, and Post Content.

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] Core data /packages/core-data [Package] Blocks /packages/blocks [Package] Block library /packages/block-library labels Aug 21, 2019
@epiqueras epiqueras added this to the Future milestone Aug 21, 2019
@epiqueras epiqueras requested a review from youknowriad as a code owner August 21, 2019 01:53
@epiqueras epiqueras self-assigned this Aug 21, 2019
@youknowriad youknowriad force-pushed the try/refactor-reusable-blocks-v3 branch from c6ca4e7 to 76f0a7b Compare August 21, 2019 08:50
@youknowriad
Copy link
Contributor

Thanks for opening the PR Enrique, I really appreciate your willingness to try different approaches.

This means we lose block navigation, simple drag and drop/moving, block inspectors, save flow/change detection UI, integrated undo UI, etc.

While this is not critical for a block with a separate save flow (reusable blocks), this definitely feels like a lot especially for a block that is meant to be saved with the same global flow. I think I agree with you that we might not want to introduce these issues. And we have some options on the table:

Do you think there's a potential approach with these in mind:

  • Separate block lists in core-data module: Why I think this is important is because I don't think we should be attaching blocks that are supposed to be saved to an entity A to an edit of an entity B. It also allow us to easily know which entity is dirty, what are the edits for a given entity...

  • Potentially a single blocklist merging all the blocklists at the "Editor" store level to be able to use it/render it/manipulate in a single BlockEditorProvider.

  • A light EntityProvider like this PR.

This also means we might have to do some tweaks to the InnerBlocks component to have a way to dispatch changes to both the entity edits (core-data) and the unified block list (editor).

What do you think of such approach?

@youknowriad
Copy link
Contributor

Oh, on another subject. I think so "split" this PR somehow, do you think we can do this in a separate PR:

  • Extract the EntiityProvider
  • Extrast the Post and PostTitle blocks (just these two)
  • Refactor the current PostTitle component in the editor moddule to use a locked instance of BlockEditorProvider in order to use this Post title instead of the current custom implementation in production today without impacting the UX entirely?

It feels like something we could try and ship quicker without going into the "template" rabbit hole yet?

@epiqueras
Copy link
Contributor Author

Thanks for opening the PR Enrique, I really appreciate your willingness to try different approaches.

😄

Separate block lists in core-data module: Why I think this is important is because I don't think we should be attaching blocks that are supposed to be saved to an entity A to an edit of an entity B. It also allow us to easily know which entity is dirty, what are the edits for a given entity...

Although I see why conceptually this seems "more correct", there would be no practical benefit over #17020. That approach of a single block list with multiple syncing levels has all the properties we need, blocks are transient edits anyway.

A light EntityProvider like this PR.

It sounds like you are describing #17020, but with useEntityProp instead of the abstract context that we let the provider/editor implementation implement.

This also means we might have to do some tweaks to the InnerBlocks component to have a way to dispatch changes to both the entity edits (core-data) and the unified block list (editor).

That's what #17020 does.

Oh, on another subject. I think so "split" this PR somehow, do you think we can do this in a separate PR:

Yeah we can do that once we agree on an approach.

@youknowriad
Copy link
Contributor

The main issues I have with #17020 are:

  • EntityHandlers: I don't think it's the responsiibility of the producer (the block using the entity handler) to define where edits will go and where blocks will go, its responsibility is just to say, inside this tree, the current entity is this thing.

  • I don't think we should be using multiple EditorProvider components. I don't see the EditorProvider as something tied to an entity, I see it as the store that orchestrate the post editor screens (with all its modes).

Although I see why conceptually this seems "more correct", there would be no practical benefit over #17020. That approach of a single block list with multiple syncing levels has all the properties we need, blocks are transient edits anyway.

I think there's a big conceptual benefit here, especially core-data is not built to serve the editor, it's built in a generic way. Imagine WordPress being an SPA and navigating to another page with lingering edits. edits for one entity woule contain things to other entities which doesn't make sense for potential usage of this store elsewhere.

I believe (If I understand properly) in #17020 we store all blocks (from all entities) in the root entity then we store the second level and all its children level in the second entity, then we store ... until we reach the leaf level right? Don't you think there's a lot of duplication here?

While in my proposal here, there's also duplication (but it's just one time) not duplicating on each sublevel.

@epiqueras
Copy link
Contributor Author

EntityHandlers: I don't think it's the responsiibility of the producer (the block using the entity handler) to define where edits will go and where blocks will go, its responsibility is just to say, inside this tree, the current entity is this thing.

We can change that and also allow having one provider per entity type like in this PR, which I think is pretty useful.

I don't think we should be using multiple EditorProvider components. I don't see the EditorProvider as something tied to an entity, I see it as the store that orchestrate the post editor screens (with all its modes).

It's useful because we can reuse a lot of the logic for syncing blocks to an entity, although after implementing the previous point, I think blocks should explicitly render the editor provider just like reusable blocks explicitly render block editor providers.

I think there's a big conceptual benefit here, especially core-data is not built to serve the editor, it's built in a generic way. Imagine WordPress being an SPA and navigating to another page with lingering edits. edits for one entity would contain things to other entities which doesn't make sense for potential usage of this store elsewhere.

Yes, but blocks are transient, not saved, so this is not affected.

I believe (If I understand properly) in #17020 we store all blocks (from all entities) in the root entity then we store the second level and all its children level in the second entity, then we store ... until we reach the leaf level right? Don't you think there's a lot of duplication here?

They are the same objects, so it's not using more memory.

I think I am leaning towards an approach like this PR except that instead of rendering a nested BlockEditorProvider, we render a modified InnerBlocks with value, onInput, and onChange props. What do you think? Yes we are "duplicating" blocks in parents, but they are references to the same objects. It's not very different to keeping them separated and then joining them, and we save on a lot of complexity that would be required to define these "join" points.

@youknowriad
Copy link
Contributor

I think I am leaning towards an approach like this PR except that instead of rendering a nested BlockEditorProvider, we render a modified InnerBlocks with value, onInput, and onChange props. What do you think?

I like that yes

Yes we are "duplicating" blocks in parents, but they are references to the same objects. It's not very different to keeping them separated and then joining them, and we save on a lot of complexity that would be required to define these "join" points.

I still don't like that, that much, I mean I can see how my proposal is a bit more complex that this but having this weird "edit" in the code-data still feels wrong. That said, we can start with the above and address that point a bit later.

@epiqueras
Copy link
Contributor Author

Ok, I'll see how this goes.

@epiqueras
Copy link
Contributor Author

Closed in favor of #17153.

@epiqueras epiqueras closed this Aug 27, 2019
@youknowriad youknowriad deleted the try/implementing-template-blocks-using-nested-block-editors 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 library /packages/block-library [Package] Blocks /packages/blocks [Package] Core data /packages/core-data
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants