Manage controlled inner blocks in content only #66753
Conversation
draganescu
added
[Type] Enhancement
|
Size Change: +489 B (+0.03%) Total Size: 1.82 MB
ℹ️ View Unchanged
|
|
I see two different behaviors depending on whether you're in "template part" focus mode or whether you're in a template containing that template part. In both I can select the navigation block but the list view is empty in one (template) but filled in the other (template part) |
|
The other difference is that in "template", I see the "navigation" block type in the inspector but in the "template-part" I see "Group". (so the section). Not sure which one should be the expected behavior, "Navigation" maybe but I'm not sure. |
draganescu
force-pushed
the
try/navigation-content-only
branch
from
360ed99 to
c44ea46
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
Flaky tests detected in 1f89550. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11709473559
|
draganescu
force-pushed
the
try/navigation-content-only
branch
from
1f89550 to
e165a1e
Compare
This comment was marked as resolved.
This comment was marked as resolved.
draganescu
force-pushed
the
try/navigation-content-only
branch
from
e165a1e to
bbc8030
Compare
draganescu
force-pushed
the
try/navigation-content-only
branch
from
ce466cc to
54f5ef2
Compare
|
I am going to solve the block inspector to show the fields to edit a navigaiton link or sumbenu. It's not clear how to filter for content but at least showing how to detect the context is good. The context means:
|
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| const { __unstableGetEditorMode } = useSelect( blockEditorStore ); | ||
| const editorMode = __unstableGetEditorMode(); |
There was a problem hiding this comment.
We shouldn't call selectors during the render.
Here, we're not subscribing to store changes; we're only getting correct editorMode values because something else is triggering a rerender on store change. That's unreliable and can lead to bugs.
| const defaultRenderingMode = useSelect( ( select ) => { | ||
| const { getBlockEditingMode } = select( blockEditorStore ); | ||
| return getBlockEditingMode( clientId ) === 'default'; | ||
| } ); |
There was a problem hiding this comment.
I think this should be available via block edit context. In that case, there's no need to create another store subscription.
|
I did a surface review, and below are a couple of things I don't like about the proposed solution.
I wonder if we can update |
|
@Mamaduka thank you for looking at this 🙏🏻 I'll explore your proposal, but I can't see immediately how to:
Anyway, first I'll iron out the code related comments. |
| export default function useListViewClientIds( { | ||
| blocks, | ||
| rootClientId, | ||
| ignoreRenderingMode, |
There was a problem hiding this comment.
Rather than adding the ignoreRenderingMode prop, you could instead pass in the getClientIdsTree blocks via the blocks prop
There was a problem hiding this comment.
Looks like doing that would end up deferring to getEnabledClientIdsTreeUnmemoized. That selector will include the block if its blockEditingMode is not disabled.
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 getEnabledClientIdsTreeUnmemoized to sometimes show all blocks but making a flag is far easier. The problem is the implementation of content only is very certain of itself and this PR fights it!
There was a problem hiding this comment.
Looks like doing that would end up deferring to getEnabledClientIdsTreeUnmemoized.
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 blockEditingMode, that doesn't feel right to me. It'll lead to having to also circumvent the block editing mode of the controlled inner blocks so that they can be selected/edited. Can't the blocks have the correct block editing mode?
There was a problem hiding this comment.
I don't understand why you want to circumvent blockEditingMode, that doesn't feel right to me. It'll lead to having to also circumvent the block editing mode of the controlled inner blocks so that they can be selected/edited. Can't the blocks have the correct block editing mode?
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. :/
| // All controlling blocks are treated as content only | ||
| // by default. | ||
| if ( areInnerBlocksControlled( state, clientId ) ) { | ||
| return 'contentOnly'; | ||
| } | ||
|
|
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 role to declare support for this "controlled content only"? role: controlled? Or is detecting controlled inner blocks reliable enough?
I think this specific conditional for Zoom Out can be easily added removed based on the required UX.
| if ( | ||
| ! isContent && | ||
| areInnerBlocksControlled( state, clientId ) | ||
| ) { | ||
| return 'contentOnly'; | ||
| } |
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 areInnerBlocksControlled as the test, because it seems to be suggesting that controlled blocks are a type of thing semantically and should all have a particular editing experience, but really whether a block is controlled or not is a part of the implementation.
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.
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).
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.
| // All controlling blocks are treated as content only | ||
| // by default. | ||
| if ( | ||
| ! isContent && | ||
| areInnerBlocksControlled( state, clientId ) | ||
| ) { | ||
| return 'contentOnly'; | ||
| } |
There was a problem hiding this comment.
There's no explanation as to how this links to templateLock, so I find it hard to understand why this check is located here. If controlled blocks are always contentOnly then maybe there could be one check right at the start of the function that returns early.
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.
only when they're inside a container which itself is a content only locking parent.
'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.
|
Updated instructions. |
| if ( areInnerBlocksControlled( state, clientId ) ) { | ||
| return 'contentOnly'; | ||
| } |
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.
There was a problem hiding this comment.
Thanks for experimenting with this. It's sometimes useful to see how these ideas play out when you actually try and use then in the Editor. That said, I'm also a little lost on the UX / Design vision for these critical interactions.
Implementing these changes is complex and time consuming and without a "north star" we could be spending a lot of time going down costly rabbit holes.
In the vein of attempting to provide something useful in this review I've taken a look specifically in the context of Navigation.
My instinct is that you should be able to make changes to the menu items in canvas. Specifically:
- text/label
- url (link)
I don't think you should be allowed to add/remove items or perform more advanced edits but basic content changes should be possible. After all, in a "simple mode" the user probably won't want to know about controlled inner blocks...etc. They just want to make some basic changes.
Deferring some responsibility to the Content List View in the inspector controls is ok, but requiring people to know about this is a step too far. After all the Write mode is supposed to be a simple experience and disabling the canvas like this will make it more complicated to undertake basic tasks like updating Nav items.
How can I help with the effort in this PR? I'd like to try and support as I recognise you've been driving solo on this.
What?
Closes #65699
Why?
Deeply nested blocks are hard to edit in design mode already. In write mode they become even more complicated to reach and reliably update.
How?
Follow-up from a discussion with @youknowriad we can update content only to:
Briefly:
Testing Instructions
"templateLock": "contentOnly"to the outermost<!-- wp:groupOpen questions
Testing Instructions for Keyboard
Screenshots or screencast
controller-content-only.mp4