-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Conversation
onChange={ this.setBlocks } | ||
onInput={ this.setBlocks } | ||
> | ||
<BlockList /> |
There was a problem hiding this comment.
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.
406774f
to
68a7d35
Compare
return false; | ||
} | ||
|
||
const referencedBlockName = getBlockName( state, reusableBlock.clientId ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
In thinking about this by my related comment at #14491 (comment) , I might have thought that the purpose of |
No I didn't consider this but I can see how it could make sense. I feel that using |
@@ -17,11 +17,15 @@ import controls from './controls'; | |||
*/ | |||
const MODULE_KEY = 'core/block-editor'; | |||
|
|||
const store = registerStore( MODULE_KEY, { | |||
export const storeConfig = { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 thecreateNamespaceStore
616544f
to
f55f6d1
Compare
04a27ce
to
d83e8fd
Compare
Good catch @mapk it should be fixed now. |
this fixes #17370 |
There was a problem hiding this 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
Let's try this. Thanks all. |
This also seems to fix #17178 🎉 |
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