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

Reusable Blocks: Reimplement reusable block as embedded editor #7119

Closed
youknowriad opened this issue Jun 4, 2018 · 8 comments · Fixed by #14367
Closed

Reusable Blocks: Reimplement reusable block as embedded editor #7119

youknowriad opened this issue Jun 4, 2018 · 8 comments · Fixed by #14367
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Type] Bug An existing feature does not function as intended
Milestone

Comments

@youknowriad
Copy link
Contributor

At the moment, fetching reusable blocks also parses their content and insert them to the editor's block list state. This approach have some drawbacks:

  • We need to maintain a cycle dependency between blocks and shared blocks
  • Fetching shared blocks from the server retriggers the parsing which means if it's currently rendered, the block will unmount/remount.

I tried simplifying the state in #7080 but it fails at dealing with the nested blocks.

The idea of the PR is to avoid having to parse the reusable blocks (content property) when loading them, and when we insert a reusable block into the editor, don’t parse it and add the parsed blocks into the editor’s state because these parsed blocks are not part of the post content but just a property of the reusable block and is already present in the state in its serialized form.

So when we insert the reusable block, its edit method is responsible of parsing the content and updating it when the user saves the reusable block’s form.

But the problem happens when we have inner blocks because the way we render the InnerBlocks we assume these are present in state, we don’t pass the list of blocks to show explicitly to the BlockList as opposed to the BlockEdit (root) where we can just pass the attributes of the block to show.

The path forward is not clear:

  • My “perfectionism” tells me we should refactor our component to avoid relying too much on state getBlock( uid ) and accepts blocks as props.
  • But this is a lot of work and we lose a lot of flexibility in using selectors etc...
  • We can also try to load another Gutenberg instance :man-shrugging: when clicking the “edit” button of a reusable block, but this obviously requires a lot of work to make Gutenberg reusable this way (which should benefit things like multiple Gutenberg instances in a page) and this also means we’d need to figure out where to show the inspector of this inner instance, should it be shown above the outer instance’s inspector.
  • Or we should keep our current hack: (load the shared blocks inner blocks into state, but try to improve it in a way that we don’t remount the shared blocks when we update them/trigger another fetch)

related #5010 #5228

@youknowriad youknowriad added [Type] Bug An existing feature does not function as intended [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) labels Jun 4, 2018
@aduth
Copy link
Member

aduth commented Jun 12, 2018

We can also try to load another Gutenberg instance :man-shrugging: when clicking the “edit” button of a reusable block, but this obviously requires a lot of work to make Gutenberg reusable this way (which should benefit things like multiple Gutenberg instances in a page) and this also means we’d need to figure out where to show the inspector of this inner instance, should it be shown above the outer instance’s inspector.

Something like this was also explored for nested blocks (i.e. idea of an embedded editor) and ultimately decided against with all the complications it proved to surface.

Related: #3745 (comment)

@aduth
Copy link
Member

aduth commented Jun 12, 2018

We need to maintain a cycle dependency between blocks and shared blocks

Is there really a dependency from blocks to shared blocks? As implemented, state.editor is meant to act as a canonical source of blocks, where shared blocks track only pointers to those blocks. It's admittedly become an issue because of change detection and because we consider since it's part of "editor" that those blocks exist in the post on the page (not always true). If we wanted the one canonical source, we could divide those in the editor to be treated as pointers as well, similar to shared blocks. I'm not sure how this works out to be able to detect when a change occurs, though.

Fetching shared blocks from the server retriggers the parsing which means if it's currently rendered, the block will unmount/remount.

Seems like there's a couple things we could be doing better here, not specific to state shape:

  • Reusing a block UID when parsing a shared block which has already been received previously
  • Not receiving (or not assigning as new state reference) a parsed shared block when it matches what's already known

@youknowriad
Copy link
Contributor Author

Just thinking in terms of modules. I believe Shared blocks belong to core-data while the editor blocks is for the post content being edited. A block is not an entity in WordPress at the moment. Merging those would make sense if the post_content was exclusively built of persisted blocks.

@aduth
Copy link
Member

aduth commented Jun 25, 2018

Related effort: #7453

@noisysocks noisysocks added this to the Future: WP 5.1 Onwards milestone Nov 5, 2018
@noisysocks noisysocks changed the title Shared Blocks: Improve the state and avoiding remounting blocks on each fetch Reusable Blocks: Improve the state and avoiding remounting blocks on each fetch Nov 5, 2018
@noisysocks
Copy link
Member

Noting for future reference that as part of this we should revert or revisit the workarounds added in #11787 and #11746.

@aduth
Copy link
Member

aduth commented Mar 22, 2019

Mind the brain dump, as I coalesce some thoughts spread across #14367, #14408, #14491

As considered at #14408 (comment) , when considering reusable blocks as an embedded editor, there are a few conflicts in the form of shared state between editors:

  • There should only be at most one selected block at a given time in an editor (including in embedded editors)
  • An editor may define a single Slot which could be intended to be filled by its own Fills, and Fills rendered in an embedded editor

For these reasons, I reflected back on nested blocks, and how there were similar early explorations of an embedded editor, until a point where it was abandoned due to the fact that inner blocks benefit from being part of the same editor state (#3745 (comment)).

Further, I contemplated on the point that an editor still needs to have some awareness of the concept of a reusable blocks in some form, in order to enable that these blocks be presented as options in various inserters. Tangentially, I wondered whether needs to be what we consider today as reusable blocks; or instead, if it could be considered as a form of "preconfigured block" (a reusable block is just core/block with a title and an initial ref attribute), or some other separate API for extending a block editor's inserter with custom items.

Considering then why we want reusable blocks as an embedded editor, there's a few existing points of redundancy we aim to eliminate:

  • Just as with any other post, a reusable block is merely a post (of type wp_block) with serialized block content. By using EditorProvider, it could benefit from leveraging all of its existing mechanisms for post saving (e.g. savePost) rather than redundant custom implementations (__experimentalSaveReusableBlock).
  • Currently, the block-editor blocks state tracks reusable blocks as a sort of immaterial data (not strictly existing as rendered in the block list), which requires various workarounds to partition real, user-manipulated blocks from those supporting the implementation of reusable blocks (example).

For the first point, it's a very valid reason for wanting to use EditorProvider, specifically in managing post parse/save lifecycle (not included with #14367). However, most of these operations aren't very difficult to recreate with a combination of parseBlocks, serializeBlocks, and some yet-to-be-introduced implementation of the corollary mutative operations to @wordpress/core-data's current fetching behaviors (i.e. "save a post" and other mutating data actions). Many other behaviors are either non-desirable or implemented custom for the reusable block (e.g. unsaved changes warning, custom messaging for the reusable blocks save success).

To the second point, I wonder if we could be doing more to remove redundancies without necessarily treating reusable block data as part of a separate editor store. For example, could we consider reusable blocks as just a container for InnerBlocks ? The problem here is that there is no inner blocks markup saved with a reusable block, to which I'd say (a) should there be, as some form of cached markup value ? and (b) could it be the responsibility of the block implementation to fetch and parse the post blocks and update itself using updateBlock to assign the inner blocks, and subsequently still return a null value for save? This may help to remove some of the redundant immaterial data we store, but conversely the fetch/update behavior still risks "dirtying" state for a non-user-interaction.

There's still the other question as well: What exactly is a reusable block, from the perspective a WordPress-agnostic block editor? The block-editor split did pretty well to keep the concept relatively abstract, though the full implementation is very tied to it being a WordPress post of type wp_block, entities which are "fetched" vs. available as part of some statically-present block registry as with the rest of block types. As noted above, I think there's maybe some potential here to explore reusable blocks as a variation of a "extended" (pre-configured) block type, perhaps even as part of the base blocks registry, further eliminating some redundancy and implementation-overawareness.

Ultimately, I think an ideal implementation would be one where the bulk of the logic was contained within a very self-sufficient block type core/block, and not treat the block type as any more special / privileged / "known" than any other. The main blocker I see to bringing this to reality is in making the preconfigured instances of this block type (i.e. the saved reusable blocks) present in the inserter in some manner.

@aduth
Copy link
Member

aduth commented Mar 22, 2019

cc @nerrad depending on your interest in the overhaul of the reusable block, as it may impact your work in #14491.

@gziolo
Copy link
Member

gziolo commented May 28, 2019

Noting, that this is highly anticipated refactor which will help to resolve the following issues:

Missing block toolbar controls for a single block:
reusable-blocks

Issues with slots and fills when multiple instances of the same reusable blocks are added to the post:
Screen Shot 2019-05-27 at 14 03 11

@noisysocks noisysocks removed their assignment May 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Type] Bug An existing feature does not function as intended
Projects
None yet
4 participants