Manage controlled inner blocks in content only #66753
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -10,22 +10,29 @@ import { useSelect } from '@wordpress/data'; | |
| import { store as blockEditorStore } from '../../store'; | ||
| import { unlock } from '../../lock-unlock'; | ||
|
|
||
| export default function useListViewClientIds( { | ||
| blocks, | ||
| rootClientId, | ||
| ignoreRenderingMode, | ||
|
There was a problem hiding this comment. Rather than adding the There was a problem hiding this comment. Looks like doing that would end up deferring to It's becoming difficult to know where the source of truth is for when a block should/shouldn't be shown. I agree with @talldan that this prop should be avoided. There was a problem hiding this comment. what @getdave says :) I tried to shape There was a problem hiding this comment.
I might be misunderstanding something, so you'd need to explain it more. Or my suggestion is being misinterpreted. To clarify - I'm suggesting doing this: const clientIdsTree = useSelect( ( select ) => getClientIdsTree( rootClientId ), [ rootClientId ] );
return (
<ListView blocks={ clientIdsTree } />
);And it looks like ListView would use the correct blocks because it has this internally: clientIdsTree: blocks ?? getEnabledClientIdsTree( rootClientId ),Having said that, I don't understand why you want to circumvent There was a problem hiding this comment.
This is the gist of what's blocking this PR and the Issue for a solution! The problem is we're fighting content only because we want a special editing UX for blocks that do not declare content role attributes to only sometimes be edtitable in certain contexts. :/ |
||
| } ) { | ||
| return useSelect( | ||
| ( select ) => { | ||
| const { | ||
| getDraggedBlockClientIds, | ||
| getSelectedBlockClientIds, | ||
| getEnabledClientIdsTree, | ||
| __unstableGetClientIdsTree: getClientIdsTree, | ||
| } = unlock( select( blockEditorStore ) ); | ||
|
|
||
| return { | ||
| selectedClientIds: getSelectedBlockClientIds(), | ||
| draggedClientIds: getDraggedBlockClientIds(), | ||
| clientIdsTree: | ||
| blocks ?? ignoreRenderingMode | ||
| ? getClientIdsTree( rootClientId ) | ||
| : getEnabledClientIdsTree( rootClientId ), | ||
| }; | ||
| }, | ||
| [ blocks, rootClientId, ignoreRenderingMode ] | ||
| ); | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3068,6 +3068,12 @@ export const getBlockEditingMode = createRegistrySelector( | |
| return 'contentOnly'; | ||
| } | ||
|
|
||
| // All controlling blocks are treated as content only | ||
| // by default. | ||
| if ( areInnerBlocksControlled( state, clientId ) ) { | ||
| return 'contentOnly'; | ||
| } | ||
|
Comment on lines
+3073
to
+3075
There was a problem hiding this comment. For me when Zoomed Out I can't access the inner blocks of sections anyway. There was a problem hiding this comment. Yes, that was added "preemptively" by me, but I don't have a case for it yet. Should probably remove it. |
||
|
|
||
|
Comment on lines
+3071
to
+3076
There was a problem hiding this comment. This is in zoomed out? I think this needs to be considered, as zoomed out currently has no content editing, and the controlled block could be buried in a section as a piece of content. There was a problem hiding this comment. Well there have been explorations where zoom out has content only edititng of sections 🤷🏻 There was a problem hiding this comment. Should we have a new I think this specific conditional for Zoom Out can be easily added removed based on the required UX. |
||
| return 'disabled'; | ||
| } | ||
|
|
||
|
|
@@ -3107,6 +3113,15 @@ export const getBlockEditingMode = createRegistrySelector( | |
| ); | ||
| const isContent = hasContentRoleAttribute( name ); | ||
|
|
||
| // All controlling blocks are treated as content only | ||
| // by default. | ||
| if ( | ||
| ! isContent && | ||
| areInnerBlocksControlled( state, clientId ) | ||
| ) { | ||
| return 'contentOnly'; | ||
| } | ||
|
Comment on lines
+3118
to
+3123
There was a problem hiding this comment. FWIW, I'm planning on moving this code to the block editor reducer soon (WIP here - #67026) I'm personally not sure about using Has there been an audit of which blocks this will affect and how it'll affect them? I think third parties could also be making controlled blocks, it's a public API as far as I know, and who knows how it's being used. There was a problem hiding this comment. The idea of the PR is to not make a solution for the navigation block, but to try and see if a blanket approach that handles all blocks with controlled inner blocks makes sense (including template parts for example). There was a problem hiding this comment.
Yes, I'm suggesting that the blanket approach doesn't make sense. For example Page List is a controlled block, so would have this treatment. Widget Area might also need to be considered. Then there's Pattern, which has it's own UI and rules for inner block editing modes and I'm concerned that this code doesn't consider it at all. I think when controlled blocks has been mentioned, what's really being talked about is a select few core entity driven blocks. |
||
|
|
||
| return isContent ? 'contentOnly' : 'disabled'; | ||
| } | ||
|
|
||
|
|
@@ -3130,6 +3145,16 @@ export const getBlockEditingMode = createRegistrySelector( | |
| select( blocksStore ) | ||
| ); | ||
| const isContent = hasContentRoleAttribute( name ); | ||
|
|
||
| // All controlling blocks are treated as content only | ||
| // by default. | ||
| if ( | ||
| ! isContent && | ||
| areInnerBlocksControlled( state, clientId ) | ||
| ) { | ||
| return 'contentOnly'; | ||
| } | ||
|
Comment on lines
+3149
to
+3156
There was a problem hiding this comment. There's no explanation as to how this links to There was a problem hiding this comment. Can't do at the start - the idea is to have the list view behavior on blocks with controlled inner blocks, when they are children of content only locked parents. Sure, I can add comments to explain. We don't want blocks with controlled inner blocks to be content only locked all the time but only when they're inside a container which itself is a content only locking parent. There was a problem hiding this comment.
'content only locking parent' can unfortunately mean quite a few things, so it's hard for me to review the code without fully understanding the context. |
||
|
|
||
| return isContent ? 'contentOnly' : 'disabled'; | ||
| } | ||
| // Otherwise, check if there's an ancestor that is contentOnly | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't call selectors during the render.
Here, we're not subscribing to store changes; we're only getting correct
editorModevalues because something else is triggering a rerender on store change. That's unreliable and can lead to bugs.