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

Try refactoring the reusable blocks to use a separate block editor #14367

Merged
merged 14 commits into from
Sep 10, 2019

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Mar 11, 2019

Closes #7119
Closes #12272
Closes #12596
Closes #15087
Closes #15979
Closes #16427
Closes #16808
Closes #17370

This is the third attempt (and hopefully the last one) to use an embeddable editor to edit the reusable blocks.

Todo

onChange={ this.setBlocks }
onInput={ this.setBlocks }
>
<BlockList />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Support for the inspector sidebar is missing.

@gziolo gziolo added the [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) label Mar 11, 2019
@gziolo gziolo requested a review from noisysocks March 11, 2019 10:37
@gziolo gziolo added [Type] Enhancement A suggestion for improvement. [Type] Code Quality Issues or PRs that relate to code quality labels Mar 11, 2019
@youknowriad youknowriad force-pushed the try/refactor-reusable-blocks-v3 branch from 406774f to 68a7d35 Compare March 12, 2019 07:39
return false;
}

const referencedBlockName = getBlockName( state, reusableBlock.clientId );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Since a reusable block can have contain blocks, no blocks or a single block, I don't think it makes sense to validate if the content is "includable" in the inserter, we should only validate if core/block is allowed.

This was inconsistent before because of the core/template trick we had.

Copy link
Member

Choose a reason for hiding this comment

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

Out of curiosity, is it something which could be extracted to its own pull request, if you're suggesting it ought not have been implemented this way in the first place?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't think it can because right now the reusable block edit function uses this parsed block with BlockEdit component to render it and update. It's very much tied to the way the reusable blocks are edited today as part of the global editor.

@aduth
Copy link
Member

aduth commented Mar 18, 2019

In thinking about this by my related comment at #14491 (comment) , I might have thought that the purpose of editor as a module would align quite well to what the implementation of the reusable block should be: coordinating an underlying block editor as being sourced from and saved to a backing post, where the post happens to be of type wp_block. Was this considered? I note here that the implementation renders BlockEditorProvider directly.

@youknowriad
Copy link
Contributor Author

Was this considered?

No I didn't consider this but I can see how it could make sense. I feel that using BlockEditor instead can be more flexible/simple but that's just a guess.

@@ -17,11 +17,15 @@ import controls from './controls';
*/
const MODULE_KEY = 'core/block-editor';

const store = registerStore( MODULE_KEY, {
export const storeConfig = {
Copy link
Member

Choose a reason for hiding this comment

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

An alternative here to exposing all the implementation details (which, on the surface, is not exceedingly worrisome) is, as in #7453, exposing a createStore which registers the store given a registry (and I suppose some optional options object for the default case in applying the persist option).

https://github.com/WordPress/gutenberg/pull/7453/files#diff-adc2d694b934c0d37f4ae99f4595afc8L26

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My ideal API is this:

const store1Config = {};
const store2Config = {};
const store1 = createStore( store1Config );
const store2 = createStore( store2Config );
const registry = createRegistry( [ store1, store2 ] );
const store3 = createStore( store2Config );
const subRegistry = createRegistry([ store1, store3 ]);

Unfortunately, this is not possible because the current createStore is dependent on the registry it's being used in. I explored whether we can completely separate the two but it's hard to do with the namespace stores.


This doesn't reply directly to your comment I guess but yeah, I feel that sharing configs is the way to go in general and registering should be part of the components mounting... The global registry makes this hard to achieve at the moment.

};
}

this.registry = createRegistry( { 'core/block-editor': storeConfig }, registry );
Copy link
Member

@aduth aduth Mar 20, 2019

Choose a reason for hiding this comment

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

Working locally in a separate branch, I observe a few more issues with the cloned registry:

  • The copied store will not include the custom middlewares we apply. As I remarked at Refactor setupEditor effects to actions #14513, we should probably plan to do away with those middlewares altogether.
  • The copied registry will not (at least with the current implementation of parent registries) include the plugins used for the parent, so generator actions (controls plugin) will fail. It's unclear to me whether we should (a) inherit plugins from a parent registry or (b) force the implementer who creates a child registry to re-declare the plugins (i.e. include a few following this.registry.use here).

If we choose to go the "B" route in the second point, it should also be noted that we couldn't pre-load the store configuration when calling createRegistry, as the control plugin is implemented to only handle controls for a store configuration after it's been included as a plugin, not retroactively.

In other words, we'd have to implement it something like:

this.registry = createRegistry( undefined, registry );
this.registry.use( controls );
this.registry.registerStore( 'core/block-editor', storeConfig );

Aside: I think it could be a reasonable enhancement for the controls implementation to retroactively handle already-registered stores, so that this wouldn't need to be written so awkwardly. It would be fine for a future change, if desirable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This just reinforces my ideas that:

  • createNamespaceStore would be better if it's independent from the registry (no need for registry argument)
  • use is not as useful as it seems and both the controls and persistence plugins should be bundled as opt-in features in the createNamespaceStore

@youknowriad youknowriad force-pushed the try/refactor-reusable-blocks-v3 branch from 04a27ce to d83e8fd Compare September 9, 2019 12:20
@youknowriad
Copy link
Contributor Author

Good catch @mapk it should be fixed now.

@senadir
Copy link
Contributor

senadir commented Sep 9, 2019

this fixes #17370

Copy link
Contributor

@senadir senadir left a comment

Choose a reason for hiding this comment

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

I've manually tested this PR against each issue it indicate and it fixed each one of them, also it close other related PRs, it works fine, I cannot see any immediate problems for now, it would close some issues that makes reusable blocks actually reusable (appending, moving, previewing).
fundamentally I think we're all approving this approach, so I'm all in into merging this and seeing what could surface later

@youknowriad
Copy link
Contributor Author

Let's try this. Thanks all.

@youknowriad youknowriad merged commit 50fb29b into master Sep 10, 2019
@youknowriad youknowriad deleted the try/refactor-reusable-blocks-v3 branch September 10, 2019 11:11
@youknowriad youknowriad added this to the Gutenberg 6.5 milestone Sep 14, 2019
@talldan
Copy link
Contributor

talldan commented Sep 23, 2019

This also seems to fix #17178 🎉

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] Code Quality Issues or PRs that relate to code quality [Type] Enhancement A suggestion for improvement.
Projects
None yet
10 participants