Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upFix: InnerBlocks should only force the template on directly set lockings #16973
Conversation
|
Howdy! During our work with Header/Footers with FSE, we experienced a bug (Automattic/wp-calypso#35429) related to adding a Cover block inside the header. When rendering the block in the page editor (inside I found this PR and ran it in Gutenberg / FSE test environment and it resolves the bug! |
| const { innerBlocks } = block; | ||
|
|
||
| this.updateNestedSettings(); | ||
| // only synchronize innerBlocks with template if innerBlocks are empty or a locking all exists | ||
| if ( innerBlocks.length === 0 || this.getTemplateLock() === 'all' ) { |
This comment has been minimized.
This comment has been minimized.
youknowriad
Aug 19, 2019
Contributor
Is getTemplateLock useful now? Should we remove inheritance entirely?
I have to admit, I'm having trouble understanding how locking in nested context work in relationship with the global template, and the template provided as prop... If I'm having trouble understanding it, others will. I'm wondering if we should have a document that clarifies how it works?
This comment has been minimized.
This comment has been minimized.
gwwar
Aug 19, 2019
Contributor
What sort of documentation would we need to unblock the PR? An example section in the handbook?
This comment has been minimized.
This comment has been minimized.
youknowriad
Aug 19, 2019
Contributor
To clarify, I don't think documentation should block the PR. I was just having hard time understanding how this work, especially thinking of all contexts. (CPT template, prop template, CPT lock, prop lock...)
This comment has been minimized.
This comment has been minimized.
jorgefilipecosta
Aug 19, 2019
Author
Member
Hi @youknowriad,
Is getTemplateLock useful now?
We only had one usage of it. I guess we can remove and inline its logic. The change was applied.
Should we remove inheritance entirely?
Nice question, I think the inheritance mechanism is useful. E.g.: without it, if I use group with some paragraphs inside or a cover in a CPT with locking all, inside the group or cover the user will always be able to do anything and locking has no effect.
If the user has a locking all and wants in a specific cover block to allow the user to do anything that's also possible nowadays. The developer can create a container block that sets locking to false and in the template set the cover to be inside that block.
So I think inheritance together with child/container blocks provides a powerful mechanism and removing inheritance is removing some options.
Yes, I agree that the logic is complex and hard to understand. I will create a separate PR that tries to document how these mechanisms work so we don't block this PR.
This comment has been minimized.
This comment has been minimized.
retrofox
Aug 19, 2019
Contributor
So With these changes, if we want to lock the synchronization we need to set it up explicitly?
<div className="wp-block-cover__inner-container">
<InnerBlocks
template={ INNER_BLOCKS_TEMPLATE }
templateLock="all"
/>
</div>Just FYI, I've been working on previewing different blocks using the <BlockPreview /> component, getting into issues specifically with core/cover and core/image-text blocks.
This PR fixes the problem.
This comment has been minimized.
This comment has been minimized.
jorgefilipecosta
Aug 20, 2019
Author
Member
So With these changes, if we want to lock the synchronization we need to set it up explicitly?
Exactly the template synchronization only happens if templateLock all is explicitly set. If the cover block was changed to that code sample the synchronization would happen.
6e173ad
to
9456479
…d all templates.
9456479
to
53085c2
This comment has been minimized.
This comment has been minimized.
|
Hi @gwwar, @noahtallen, @youknowriad, thank you for checking/reviewing this PR. |
|
I see no reason to block this PR more. Thanks Jorge |



jorgefilipecosta commentedAug 8, 2019
Supersedes: #16882
Currently, InnerBlocks forces the template when locking is all even if the locking is inherited.
This logic is not correct, because if block used in a template contains another template, the inner blocks for that block set on the parent template will be ignored as the child will inherit the locking all and the template of the child will start to be forced.
This PR updates the InnerBlocks logic to only force the template if the locking all is directly set and not inherited. In cases, the locking all is inherited it should be the template of the parent managing the inner blocks.
How has this been tested?
I pasted the following code in functions.php:
I created a new Locked All Post and I verified the InnerBlocks set for media-text and cover appear as expected.
Props to @oxyc for bringing this issue for our attention and proposing an initial fix for the media text-specific case.