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

Fix: InnerBlocks should only force the template on directly set lockings #16973

Conversation

@jorgefilipecosta
Copy link
Member

commented Aug 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:


register_post_type(
	'locked-all-post',
	array(
		'public'        => true,
		'label'         => 'Locked All Post',
		'show_in_rest'  => true,
		'template' => [
			['core/media-text',
				[
					'mediaPosition' => 'right',
					'mediaId' => 80,
					'mediaType' => 'image',
					'imageFill' => false,
					'mediaUrl' => 'http://localhost/app/uploads/2019/07/thumb-1463-product-primary-desktop.png',
					'className' => 'is-style-hero',
					'backgroundMediaId' => 89,
					'backgroundMediaUrl' => 'http://localhost/app/uploads/2019/07/mutti-polpa-zoom-copy.jpg',
				], [
					['core/heading', ['align' => 'right', 'level' => 1, 'placeholder' => __('Product name', 'mutti')]],
					['core/heading', ['align' => 'right', 'level' => 4, 'placeholder' => __('Descriptive product name', 'mutti')]],
					['core/paragraph', ['align' => 'right', 'placeholder' => __('A short description about the product and why it is unique.', 'mutti')]],
				],
			],
			['core/cover',
				[
					'url' => 'http://yah.local/wp-content/uploads/2019/08/Jul-14-2019-13-21-12-16.gif',
				],
				[
					['core/heading', ['align' => 'right', 'level' => 1, 'placeholder' => __('Product name', 'mutti')]],
				],
			]
	],
		'template_lock' => 'all',
	)
);

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.

Copy link
Contributor

left a comment

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 Disabled and with the editor settings having templateLock='all'), several of the cover's children blocks would not be rendered. I traced it back to InnerBlocks, where it was synchronizing the blocks with the template and replacing most of the content.

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.

Copy link
@youknowriad

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.

Copy link
@gwwar

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.

Copy link
@youknowriad

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.

Copy link
@jorgefilipecosta

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.

Copy link
@retrofox

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.

Copy link
@jorgefilipecosta

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.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/inner-blocks-should-only-force-template-on-explicit-lockings branch from 6e173ad to 9456479 Aug 19, 2019
@jorgefilipecosta jorgefilipecosta force-pushed the fix/inner-blocks-should-only-force-template-on-explicit-lockings branch from 9456479 to 53085c2 Aug 19, 2019
@jorgefilipecosta

This comment has been minimized.

Copy link
Member Author

commented Aug 19, 2019

Hi @gwwar, @noahtallen, @youknowriad, thank you for checking/reviewing this PR.
I performed a small update and I will try to create a parallel PR with documentation.

Copy link
Contributor

left a comment

I see no reason to block this PR more. Thanks Jorge

@jorgefilipecosta jorgefilipecosta merged commit 9c9fba9 into master Aug 20, 2019
1 of 4 checks passed
1 of 4 checks passed
Filter opened Filter opened
Details
Filter opened Filter opened
Details
Milestone It Milestone It
Details
Travis CI - Pull Request Build Passed
Details
@jorgefilipecosta jorgefilipecosta deleted the fix/inner-blocks-should-only-force-template-on-explicit-lockings branch Aug 20, 2019
donmhico added a commit to donmhico/gutenberg that referenced this pull request Aug 27, 2019
dd32 pushed a commit to dd32/gutenberg that referenced this pull request Sep 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.