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

Allow changing Block attributes dynamically in InnerBlocks template. #16244

Closed
wants to merge 2 commits into from
Closed

Allow changing Block attributes dynamically in InnerBlocks template. #16244

wants to merge 2 commits into from

Conversation

desaiuditd
Copy link
Member

Description

<InnerBlocks> doesn't seem to respect dynamic changes in its template while re-rendering the component. It stays on its initial state when it's rendered for the first time. Then on wards, if the block attributes in the template are changed, it does not update them in the blocks within.

On further investigation, it was found that <InnerBlocks> component only synchronizes its block with the given template, when templateLock === 'all'.

And further synchronizeBlocksWithTemplate method from @wordpress/blocks package only re-creates the block, if the order of that block is changed in the given template. Check here

So if a consumer compoent of <InnerBlocks /> is passing a dynamic template where the attributes (such as content, placeholder etc.) are changing in the template, then those changes won't ever come into effect. The <InnerBlocks /> will always render the template correctly first time. Not subsequently.

I've created an example block where I'm creating the same use-case and it's not working as expected in the master branch of Gutenberg. Example

This PR fixes the issue.

How has this been tested?

  • I've added unit tests to support my use-case.
  • My change may also affect <BlockList> component or Group Blocks. Not really sure at this stage, as I've not thoroughly tested other components. However, I'm trusting the unit tests at the moment. Happy to dive deep, if needed.

Screenshots

Types of changes

  • Code change in the Blocks API function - synchronizeBlocksWithTemplate to allow dynamic templates in the <InnerBlocks> component.
  • At this state, this seems like a new feature, because currently <InnerBlocks> doesn't allow changing the templates dynamically.
  • I don't believe, this is a breaking change. I've tried to cover all the possibilities around that function based on the existing unit tests. But bear with my ignorance here.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@gziolo gziolo added First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P [Feature] Block API API that allows to express the block paradigm. labels Jun 24, 2019
// either the template is given without any block attributes (this means the template doesn't care about the content).
! attributes ||
// or the attributes are same as what is given in the template.
isShallowEqual( attributes, block.attributes )
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the issue is that some templates do define attributes as initial value and this would break that behavior.

Copy link
Member Author

Choose a reason for hiding this comment

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

If the template is defining the attributes then they will be overridden in the newly created block below. So should be fine. Isn't it?

Maybe I'm missing the point. Can you give me an example of it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Say I have this template:

const template = [
  [ 'core/paragraph', { 'content', 'some initial random content' } ],
]

If I'm understanding the change here, everytime the post using this template as locked will be loaded, it will reset the content of the paragraph to "some initial random content". Am I wrong?

Copy link
Member Author

Choose a reason for hiding this comment

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

You’re right.

Are you raising concern about backward compatibility?

If the template is changed midway, then the existing content will go away. But isn’t it that case as well, when the order of blocks is changed in the template? That’s what this unit test is suggesting.

My understanding is, if the template is used with locking, then it’s implicit that whatever the template is forcing, it will be applied, irrespective of the existing content.

I’ve attached a screencast of what I’m trying to achieve. That may help in understanding my use case.

2019-06-27 12 21 56

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, that's my concern. This PR is basically changing the meaning of the templateLock="all". It was always meant to be something used to lock the block used but not their content even if you provide initial content.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay. Then what do you have to say about this? 👇🏼

But isn’t it that case as well, when the order of blocks is changed in the template? That’s what this unit test is suggesting.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, locking locks the order as well. It's how it was designed. For example you want to render a list of blocks in a product template with a given order, lock everything and only allow the user to edit the content.

Copy link
Member Author

@desaiuditd desaiuditd Jul 20, 2019

Choose a reason for hiding this comment

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

Sorry, I'm coming back to this after some time. But just not able to get my head around the current behaviour.

You said that we should not change any existing content of any blocks irrespective of templateLock is applied or not.

But that's already happening if the order of blocks within a template is changed.

E.g.,

// Observe the order of blocks is changed, keeping everything else same.
const templates = [
	[
		[
			'core/heading',
			{ content: name }
		],
		[
			'core/quote',
			{ value: company }
		],
		[
			'core/paragraph',
			{ content: `The balance is - ${ balance }` }
		]
	],
	[
		[
			'core/quote',
			{ value: company }
		],
		[
			'core/paragraph',
			{ content: `The balance is - ${ balance }` }
		],
		[
			'core/heading',
			{ content: name }
		]
	],
];

const template = templates[ Math.floor( Math.random() * Math.floor( 2 ) ) ];

const ALLOWED_BLOCKS = [ 'core/paragraph' ];

return (
	<InnerBlocks
		template={ template }
		templateLock="all"
		allowedBlocks={ ALLOWED_BLOCKS }
	/>
);

Above snippet is changing the blocks along with the content in v6.1.1

Isn't that contradictory? If not, then my understanding of allowing the template to override the content in any case is valid.

@desaiuditd
Copy link
Member Author

Declining in favor of #16678

@desaiuditd desaiuditd closed this Jul 24, 2019
@desaiuditd desaiuditd deleted the add/allow-changing-block-attributes-dynamically-in-inner-blocks-templates branch July 24, 2019 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Nested / Inner Blocks Anything related to the experience of nested/inner blocks inside a larger container, like Group or P First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants