Add InnerBlocks locking #6991

jorgefilipecosta · 2018-05-28T18:45:43Z

Description

The feature is required by #6993.
This PR adds the possibility to set locking in InnerBlocks in a similar way we can now do with the template locking.
Usage:

edit() {
	return el( 'div', { style: { outline: '1px solid gray', padding: 5 } },
		el( InnerBlocks, {
			locking: 'insert', // all, insert, or none
			template: [
				[ 'core/paragraph', {
					placeholder: 'Enter locking insert p1 content…',
				} ],
				[ 'core/paragraph', {
					placeholder: 'Enter locking insert p2 content…',
				} ],
			],
		} )
	);
}

Locking can be all, insert or not set.
Right now, with template locking if we have locking active and a template with a column block we cannot insert anything into the column so it seems locking applies to children.
We followed the same logic in this PR locking applies to descends even if the descends set a less restrictive locking or no locking at all. We may change this behavior but if changing it we should also change it for the CPT templates case.

How has this been tested?

The best way is to use the test blocks in https://gist.github.com/jorgefilipecosta/2bac096fe3be7f6ff0de7452292161cb, and verify everything works as expected. To simplest way to have the blocks is to paste all the code in the developer console.

youknowriad · 2018-05-29T11:00:44Z

editor/utils/block-list.js

+			static getDerivedStateFromProps( props ) {
+				const newSettings = {
+					allowedBlocks: props.allowedBlocks,
+					locking: getHighestLocking( props.locking, props.parentLocking ),


Why do we compare the locking prop with the parent locking prop? Shouldn't we just always use the prop passed to InnerBlocks?

Hi @youknowriad, this is what allows us to propagate the locking to descendants. Even if a child block sets no locking at all or a less restrictive locking the locking of the parent applies. So for example, if the parent adds locking and uses columns block the locking applies also to columns. I'm not certain if this is the right behavior and in fact, I'm now considering changing to just propagate the locking if the child block did not set locking at all.

yes, I'd consider props.locking || props.parentLocking a better approach here :) (or something in that vein. We could also question whether propagation is something we want (I know we do this at the moment)

youknowriad · 2018-05-29T11:00:57Z

editor/store/selectors.js

 			return false;
 		}

 		const parentBlockListSettings = getBlockListSettings( state, parentUID );
-		const parentAllowedBlocks = get( parentBlockListSettings, [ 'supportedBlocks' ] );
+		const parentAllowedBlocks = get( parentBlockListSettings, [ 'allowedBlocks' ] );


Is this like an unrelated fix?

Oh I see, it's renamed nvm

youknowriad · 2018-05-29T11:02:52Z

I'm glad to see this being worked on. I wonder if we should support setting this prop in the "template" as well? Like we currently do for the children blocks.

tofumatt

Maybe I'm missing something but the documentation and usage of the getLocking function seems very inconsistent. I'm not quite following it.

tofumatt · 2018-05-29T11:02:15Z

editor/store/selectors.js

@@ -1744,3 +1744,21 @@ export function getSupportedBlocks( state, uid, globallyEnabledBlockTypes ) {
 export function getEditorSettings( state ) {
 	return state.settings;
 }
+
+/*
+ * Returns the editor settings.


This comment seems inaccurate; maybe it was a copy/paste oversight?

This seems to just return the locking setting for this block.

True I totally missed the update to the comment. Thank you for the catch.

tofumatt · 2018-05-29T11:03:05Z

editor/store/selectors.js

+ *
+ * @param {Object} state Editor state.
+ *
+ * @return {Object} The editor settings object


This appears to return the locking setting, which is a string (with particular values). Looks like this needs updating.

tofumatt · 2018-05-29T11:03:55Z

editor/store/selectors.js

+/*
+ * Returns the editor settings.
+ *
+ * @param {Object} state Editor state.


This takes two arguments but only one is listed.

tofumatt · 2018-05-29T11:04:46Z

editor/store/selectors.js

+ *
+ * @return {Object} The editor settings object
+ */
+export function getLocking( state, rootUID ) {


I see this function being called using getLocking( rootUID ) a few times. Am I misunderstanding something?

The state is automatically injected by the data module

(see the README here https://github.com/WordPress/gutenberg/tree/master/data)

Oh, okay. That is really hard to grok from reading the code 😢

tofumatt · 2018-05-29T11:05:30Z

editor/components/block-drop-zone/index.js

 		return {
-			isLocked: !! templateLock,
+			isLocked: !! getLocking( rootUID ),


The function signature for getLocking looks like getLocking( state, rootUID )… seems like this is wrong.

tofumatt · 2018-05-29T11:06:51Z

editor/store/selectors.js

+	}
+	const blockListSettings = getBlockListSettings( state, rootUID );
+	if ( ! blockListSettings ) {
+		return;


Why return void? Wouldn't it make sense to at least return null? It seems more semantically valid...

tofumatt · 2018-05-29T11:11:21Z

Looks like the docs for getLocking are still off, but I see now I didn't understand how the data module worked.

mtias · 2018-05-30T14:09:15Z

editor/store/selectors.js

+ *
+ * @return {?string} Locking in the context of a given block.
+ */
+export function getLocking( state, rootUID ) {


This name feels a bit odd to me. What can be the locked states returned in the string?

The locking states can be all, insert or undefined/none. Maybe getLockingState is better alternative?

Yes, shoot, I meant to mention this in my review and knew I was forgetting something.

getLockingStatus or getLockSetting or something similar would be better. getLocking feels incomplete to me as well.

That would be more clear, yes: getLockedState. The other concern is that it seems slightly backwards to consider the locked state as the property when most blocks are going to be unlocked.

mcsf · 2018-05-30T17:30:06Z

editor/components/block-list/insertion-point.js

@@ -92,13 +92,13 @@ export default compose(
 		);

 		return {
-			templateLock: getEditorSettings().templateLock,
+			locking: getLockedState( insertionPoint.rootUID ),


Prop locking should also have its name updated in accordance with getLockedState, or should be renamed entirely to something like isLocked with isLocked: !! getLockedState( … )

isLocked is much nicer, I vote for that.

Naming was updated.

At the editor level, we name template locking templateLock

At the block level, we name template locking lock

mcsf · 2018-05-30T17:34:49Z

editor/components/inner-blocks/index.js

-function InnerBlocks( { BlockList, layouts, allowedBlocks, template } ) {
-	return <BlockList { ...{ layouts, allowedBlocks, template } } />;
+function InnerBlocks( { BlockList, layouts, allowedBlocks, locking, template } ) {
+	return <BlockList { ...{ layouts, allowedBlocks, locking, template } } />;


See previous comments on naming.

I changed the prop to "lock", in templates we use template_lock, so I think using "lock" here is consistent. Unfortunately using isLocked in this case is not possible because we are not setting a boolean value.

I changed the prop to "lock", in templates we use template_lock, so I think using "lock" here is consistent. Unfortunately using isLocked in this case is not possible because we are not setting a boolean value.

But in the context of InnerBlocks, isn't the locking still referring to the template?

mcsf · 2018-05-30T17:38:22Z

editor/store/selectors.js

+ * @return {?string} Locked state in the context of a given block.
+ */
+export function getLockedState( state, rootUID ) {
+	if ( ! rootUID ) {


Should we make rootUID an explicitly optional param so we can avoid the check for falsy values?

getLockedState( state, rootUID = false ) { if ( rootUID === false ) {

Hi @mcsf,
I followed your suggestion with a small change:

export function getLockedState( state, rootUID = null ) { if ( rootUID === null ) {

Sometimes we use other selectors to know the rootUID and they may return null. In that case, we should consider that no rootUID was passed. Using this approach we can avoid extra checks before calling this selector (making the code simpler).

I had to revert this change and use the false check because I noticed a regression. Some selectors may return an empty string if rootUID is not set e.g. getBlockRootUID( uid ) returns an empty string for top-level blocks.
The best/simplest way to check for undefined, null or empty string seems to be a false check.

mcsf · 2018-05-30T17:41:02Z

editor/utils/block-list.js

 				return {
 					block: getBlock( uid ),
 					blockListSettings: getBlockListSettings( uid ),
+					parentLocking: getLockedState( getBlockRootUID( uid ) ),


See previous comments on naming.

mcsf · 2018-05-30T17:51:10Z

editor/utils/block-list.js

+				};
+				if ( ! isShallowEqual( props.blockListSettings, newSettings ) ) {
+					props.updateNestedSettings( newSettings );
+				}


On usage of getDerivedStateFromProps:

Strictly speaking, it should return null instead of undefined.

Side effects are to be avoided, and that is exactly what this particular method is doing. :)

Hi @mcsf,
You are right it looks like getDerivedStateFromProps should be a pure function without side effects.
I updated the code to use componentDidUpdate as going back to componentWillReceiveProps is not a long-term option. componentDidUpdate also does not seems like a perfect solution as we may have double render executions but it seems the best option we have and according to the react docs users don't see the intermediate dom state.

aduth · 2018-06-20T20:33:57Z

Would it be reasonable to represent inner block locking by allowedBlocks={ [] } ?

jorgefilipecosta · 2018-06-21T07:38:48Z

Would it be reasonable to represent inner block locking by allowedBlocks={ [] } ?

There are two types of locking "all" and "insert" and this syntax does not allow to represent both types. Besides that having allowedBlocks={ [] } does not in fact prevent from inserting other blocks inside, because according to our API allowedBlocks does not prevent children from being inserted. In fact I expect many blocks setting allowedBlocks={ [] } while having many child blocks that we can insert inside.

jorgefilipecosta · 2018-06-27T14:48:23Z

This PR was rebased and most of the feedback are addressed. It is ready for a new look :)

aduth · 2018-06-30T21:41:02Z

editor/components/inner-blocks/index.js

@@ -43,15 +43,19 @@ class InnerBlocks extends Component {
 	updateNestedSettings() {
 		const {
 			blockListSettings,
-			allowedBlocks: nextAllowedBlocks,
+			allowedBlocks,
+			lock,


Mentioned at #6991 (comment): If we call this templateLock at the editor level, why do we not call it the same here?

My idea was to try to move locking out of being a template concept. I wanted the lock to be a setting blocks can make use. E.g. have settings that enable or disable locking.
But yes given that now its main use case is templates I updated the name to be templateLock.

aduth · 2018-06-30T21:41:56Z

editor/components/inner-blocks/index.js

-			allowedBlocks: nextAllowedBlocks,
+			allowedBlocks,
+			lock,
+			parentLock,


Where is parentLock being passed?

Sorry I missed the commit of parentLock retrieval during the rebase. It was corrected.

aduth

Works well in testing

aduth · 2018-07-02T14:23:34Z

editor/store/selectors.js

+	if ( ! rootUID ) {
+		return state.settings.templateLock;
+	}
+	const blockListSettings = getBlockListSettings( state, rootUID );


There aren't any unit tests for this new behavior.

Unit tests were added.

aduth · 2018-07-02T14:24:56Z

editor/components/inner-blocks/index.js

-			} );
+		const newSettings = {
+			allowedBlocks,
+			templateLock: templateLock === undefined ? parentLock : templateLock,


Should it be the responsibility of someone rendering a BlockList to pass the parentLock, or should that component be smart enough to inherit?

If a parent block has locking but the child does not, which one wins?

When rendering a BlockList we don't pass the parentLock. We pass the templateLock. parentLock is just used for the case where templateLock is undefined and in that case we store parentLock in templateLock. Besides that case we never use parentLock again.

If a parent block has locking but the child does not, which one wins?

If the locking of an InnerBlocks area is undefined we use the locking of the parent. In all other cases, we use the templateLock passed even if the locking of the child is more permissive than the locking of the father.
Before we used always the less permissive locking of the tree #6991 (comment) but it was changed.

Dismissing because I think the points were already addressed. If there is anything I missed, feel free to comment.

jorgefilipecosta added the [Type] Enhancement A suggestion for improvement. label May 28, 2018

jorgefilipecosta self-assigned this May 28, 2018

jorgefilipecosta requested review from noisysocks and a team May 29, 2018 10:52

youknowriad reviewed May 29, 2018

View reviewed changes

tofumatt previously requested changes May 29, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/inner-blocks-locking branch from 15d5c50 to 4487774 Compare May 29, 2018 11:29

mtias reviewed May 30, 2018

View reviewed changes

mcsf reviewed May 30, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/inner-blocks-locking branch 2 times, most recently from a281470 to 28477cf Compare June 1, 2018 15:47

jorgefilipecosta force-pushed the add/inner-blocks-locking branch from 28477cf to 03123ee Compare June 19, 2018 16:21

This was referenced Jun 20, 2018

Inner Blocks: Refactor treatment of block settings #7418

Merged

Support wrapping grouped layouts in InnerBlocks #6895

Closed

jorgefilipecosta force-pushed the add/inner-blocks-locking branch 2 times, most recently from aabb66b to 8812b72 Compare June 27, 2018 14:34

jorgefilipecosta mentioned this pull request Jun 29, 2018

Blocks: Render columns as nested column (wrapper) #7234

Merged

jorgefilipecosta force-pushed the add/inner-blocks-locking branch 2 times, most recently from 6c2966b to a136dfe Compare June 30, 2018 11:47

aduth reviewed Jun 30, 2018

View reviewed changes

jorgefilipecosta force-pushed the add/inner-blocks-locking branch 2 times, most recently from 14a759e to 9bbccb8 Compare July 2, 2018 11:15

aduth approved these changes Jul 2, 2018

View reviewed changes

Added InnerBlock locking mechanism.

50ccd28

jorgefilipecosta force-pushed the add/inner-blocks-locking branch from 9bbccb8 to 50ccd28 Compare July 2, 2018 15:59

jorgefilipecosta merged commit 060fb5f into master Jul 2, 2018

mtias added this to the 3.2 milestone Jul 5, 2018

youknowriad deleted the add/inner-blocks-locking branch July 12, 2018 10:02

Add InnerBlocks locking #6991

Add InnerBlocks locking #6991

Conversation

jorgefilipecosta commented May 28, 2018 • edited Loading

Description

How has this been tested?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

youknowriad commented May 29, 2018

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt commented May 29, 2018

Choose a reason for hiding this comment

jorgefilipecosta May 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta Jun 1, 2018 • edited Loading

Choose a reason for hiding this comment

aduth commented Jun 20, 2018

jorgefilipecosta commented Jun 21, 2018

jorgefilipecosta commented Jun 27, 2018

Choose a reason for hiding this comment

jorgefilipecosta Jul 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgefilipecosta commented May 28, 2018 •

edited

Loading

jorgefilipecosta May 30, 2018 •

edited

Loading

jorgefilipecosta Jun 1, 2018 •

edited

Loading

jorgefilipecosta Jun 1, 2018 •

edited

Loading

jorgefilipecosta Jul 2, 2018 •

edited

Loading