Allow using multiple controlled inner blocks refering to the same entity #27885
Conversation
youknowriad
added
[Type] Bug
| @@ -113,6 +120,37 @@ export default function useBlockSync( { | |||
| onChangeRef.current = onChange; | |||
| }, [ onInput, onChange ] ); | |||
|
|
|||
| // Determine if blocks need to be reset when they change. | |||
There was a problem hiding this comment.
The changes after this line are not necessary (just reordering the hooks). I believe this order makes more sense. "External" then "Internal" changes.
There was a problem hiding this comment.
Actually it seems the reordering has an effect, without it, this PR doesn't work :)
|
Size Change: +36 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
| @@ -96,8 +98,18 @@ export default function useBlockSync( { | |||
| if ( clientId ) { | |||
| setHasControlledInnerBlocks( clientId, true ); | |||
| __unstableMarkNextChangeAsNotPersistent(); | |||
| replaceInnerBlocks( clientId, controlledBlocks ); | |||
| // This needs to be called after the setHasControlledInnerBlocks call | |||
There was a problem hiding this comment.
Good catch, this is actually not needed anymore, it's an artifact of one of my earlier explorations.
|
Going to merge this. I want to follow-up with a few things here:
|
|
This is very exciting to see, great work 👏 |
|
Santa was some hours early this year. |
|
Thanks for getting this fix in @youknowriad! It's very similar to one of my earlier explorations, which would have been way easier had I known about the |
|
This breaks navigation and widgets because both of those screens maintain a mapping of block client IDs to widget / menu item IDs. Cloning the block changes the client ID which makes the screen unable to tell which widget / menu item needs to be updated. Is there an alternative way to fix this? |
Can't think of any to be honest, we tried several approaches with no luck.
Shouldn't the menuId/widgetId be an attribute of the blocks them selves? |
|
Possibly we could avoid cloning the block in the case that there are no other things referencing the same entity. (I think now it clones every time?) That might solve it for widgets + navigation, unless those screens have a possibility of showing the same entity in more than one place |
|
@noahtallen true but it seems that we'll be making the screens dependent on an implementation detail. It should be made clear whether the "block clientIds" you supply could be changed or not by the editor. I lean towards yes since it's not really the only place where the ids can be swapped. For instance, hitting "Enter" at the end of a paragraph actually changes the id of that paragraph. |
Wouldn't this mean that every block has to have a
I disagree that client IDs are an implementation detail. We expose them in several selectors ( |
I'm not really sure how the widgets and navigation screen works tbh to be able to give the right reply here. In my mind (originally), the widget area as a whole is a single "widget" (at least that's how it used to work originally) so it would get a widgetId. If it's different now each block is a widget, then yes, the ones that are widgets should probably get a widget block wrapper or something referring to the corresponding widget, don't you think?
Client IDs are internal to the block editor store, they are APIs inside the block-editor package but I don't think we ever had the expectation that clientIds don't change from an external API point of view. In fact, they are considered "temporary" (not persisted) from the start so subject to change. |
@youknowriad I had the same thought separately (link to the slack convo - https://wordpress.slack.com/archives/C01D71823PB/p1610003184385900), but there are still a few challenges with that:
I don't know if we'd consider adding extra properties to attribute declarations like follows to solve those issues: "widgetId": {
"type": "string",
"isSerializable": false, // not serialized to HTML.
"isDuplicable": false // ignored by `duplicateBlock`.
} |
|
You're right, that's good, one less thing to worry about. |
closes #22639
This uses a very simple trick to solve the duplicated controller inner blocks bug (duplicate a template part and edit one of them and see it reflected properly).
The main issue was that the block-editor is meant to only work with unique block ids, the block tree can't contain duplications. To solve this, the trick used in this PR is that for each "controlled inner blocks", instead of editing the controlled blocks directly, clone these blocks when there's an "external" change for these blocks (initialization, changing coming from a duplicated block...) and only pass to the editor clones which means the IDs are different.
Now, this can create infinite loops: an edit in one block, is going to cause changes in the other blocks too. We need to stop the edits in the second block from triggering back an edit on the original block. Fortunately, this is already handled in useBlockSync by saying: "The first change after an external change is ignored.
Also, there might be a small performance impact: All the other duplicated blocks will remount (all blocks change) on a change happening on one of them. That said, I believe this impact is acceptable for two reasons: