Allow template part editing in write mode #67372
Conversation
talldan
added
[Type] Enhancement
|
Size Change: +279 B (+0.02%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 00bc8ae. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/12270164427
|
| } | ||
|
|
||
| // Allow selection of template parts outside of sections. | ||
| if ( blockName === 'core/template-part' ) { |
There was a problem hiding this comment.
I'm curious - are we "allowed" to reference core blocks like this in the block editor package?
If not how can we pass in references to these blocks and express the custom logic here that relates to "template parts"?
I was thinking of a private block editor setting, but then I'm not sure how we'd handle the custom logic that seems to relate specifically to template parts.
I'm also thinking about how we could extend this to the Navigation block in the future.
There was a problem hiding this comment.
I think not ideal to have loads of this, but it's ok for testing new concepts. I don't think it's harmful to any non-WP consumers of the block editor package as these bits of code will have no effect if they don't have these block types. It's just a bit of a code quality issue to have lots of hard to follow conditions, and it's better in the long run to try unifying some concepts behind an API or similar, but sometimes the right one takes a while to emerge.
Hopefully someday in the future template parts will be deprecated and there will only be synced patterns, and they'll work the same way.
There was a problem hiding this comment.
We do have a fair bit of this already in the block editor package 😅
Could we use a similar logic to get/set groupingBlockName for the template part block? Is it likely that third parties will provide other types of template part-like blocks that we might want this code to work with?
There was a problem hiding this comment.
core/block is a special block (We could even relocate it within block-editor). Template part should be just core/block ultimately. I think we should try to avoid any other block type (maybe other than these two)
|
Thank you so much for all your work here! Some quick feedback after going through it today with some folks:
|
We actually do. When a template is rendered when in “Design” mode, you can select the header and see “Edit”. But in this scenario I’m referring to, the “Edit” button opens the template part in an isolated view. I propose we do the same here, treating them the same. |
That might be from this bug in
So retain the 'Edit' button? Synced patterns are also another case, 'Edit original' still shows in Write Mode when they're used as sections. Either way, the change is small enough that it could be considered for a follow-up that adjusts how this works across a wider range of blocks. |
I'm not sure what @richtabor means here but I can relay the feedback I've heard in chatting with a few folks last week that the "edit" button didn't make sense here in write mode in the block toolbar for template parts. I think as part of simplifying the site editing experience, we should remove the "edit" option when in Write mode in order to keep someone on the main canvas. It's an odd option to have listed when so few options are available and too prominent as is. |
It was pretty easy to git blame this, but I still didn't find a clear answer. The
I'd forgotten the first one exists. I don't think I've seen a use case for it. In terms of answering your question, my hunch is:
|
talldan
force-pushed
the
add/write-mode-section-editing-to-template-parts
branch
from
1fd035e to
b15f5e3
Compare
|
I think there's one more blocker issue for this, which is that when editing a Page in the Site Editor (in Template Locked rendering mode), template parts are now editable (including the inner blocks). Steps to Repro
In this PR: Template parts and their inner blocks can be edited I'll be meditating on how to solve this! It's challenging because the Zoom Out / Write mode block editing modes currently take priority over the Template Locked block editing modes. Zoom Out and Write Mode are also implemented in the block editor package, where there's no knowledge that Template Locked rendering mode exists, as that's a WordPress concept, so there's no easy option for coordinating between the two. I'm wondering if the block editing mode priorities should change so that it's still possible to call |
talldan
force-pushed
the
add/write-mode-section-editing-to-template-parts
branch
2 times, most recently
from
46a9999 to
cfbf48a
Compare
…te part content blocks
…derived mode First iteration of handling disabled blocks as derived block editing modes
talldan
force-pushed
the
add/write-mode-section-editing-to-template-parts
branch
from
cfbf48a to
631ac3a
Compare
|
I think I've got this working as is expected now, but I'm not so happy with the implementation and the performance tests don't seem happy, so I'll need to revise a few things. In terms of the implementation, I don't like how |
| // Otherwise, check if there's an ancestor that is contentOnly | ||
| const parentMode = getBlockEditingMode( state, rootClientId ); | ||
| return parentMode === 'contentOnly' ? 'default' : parentMode; | ||
| return 'default'; |
There was a problem hiding this comment.
I personally found this code really confusingly written (and the comment made me more confused). It'd be easier to understand if it were written like this:
const parentMode = getBlockEditingMode( state, rootClientId );
// Child blocks inherit a 'disabled' state from their parents.
if ( parentMode === 'disabled' ) {
return 'disabled';
}
// Other modes are not inherited, return the 'default' value.
return 'default';That would make it clear that the part I've refactored is the if ( parentMode === 'disabled' ) block. It has now moved to the withDerivedBlockEditingModes reducer.
There was a problem hiding this comment.
I'm having a hard time connecting your comment with the existing code and why the change? (Agreed it's confusing)
| if ( hasDisabledBlocks ) { | ||
| const ancestorBlockEditingMode = getAncestorBlockEditingMode( | ||
| state, | ||
| clientId | ||
| ); | ||
| if ( ancestorBlockEditingMode === 'disabled' ) { | ||
| derivedBlockEditingModes.set( clientId, 'disabled' ); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is the part I've refactored from the getBlockEditingModes selector.
After this change, if a parent (ancestor) block has a blockEditingMode of 'disabled', the children will have a disabled derivedBlockEditingMode to reflect the way the mode is inherited by children.
The selector already has code to check derivedBlockEditingModes and return the value from it.
| // If the block already has an explicit block editing mode set, | ||
| // don't override it. | ||
| if ( state.blockEditingModes.has( clientId ) ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
I think this change might need some discussion. Previously, if a block had a blockEditingMode explicitly set to something (say 'disabled' or 'contentOnly'), but you then switched to zoom out, write mode, or if the block is in a synced pattern, then those experiences would be able to override the blockEditingMode (they would set modes within the derivedBlockEditingModes state and that had precedence over blockEditingModes).
This change makes it the other way around, the explicit blockEditingModes now has precedence.
It's perhaps questionable because it allows plugins to change how those features work by calling setBlockEditingMode.
On the other hand, it's what allows Page Editing to work correctly in Write Mode, the editor package calls setBlockEditingMode in its DisableNonPageContentBlocks component to control certain aspects of the experience. The editor is really an extensibility layer over the block editor package, I haven't been able to think of a way to give it special privileges over the block editing modes without also opening up the access to plugins (other than introducing another private API).
There was a problem hiding this comment.
This change makes it the other way around, the explicit blockEditingModes now has precedence.
I think this makes more sense, that said, I think we should work to remove/deprecate the explicit blockEditingModes entirely (not specifically here) because that is just creating confusion and moving us away from decorativeness.
| derivedBlockEditingModes: new Map( [ | ||
| [ '6cf70164-9097-4460-bcbf-200560546988', 'disabled' ], | ||
| [ 'ef45d5fd-5234-4fd5-ac4f-c3736c7f9337', 'disabled' ], | ||
| [ 'b26fc763-417d-4f01-b81c-2ec61e14a972', 'disabled' ], | ||
| [ '9b9c5c3f-2e46-4f02-9e14-9fe9515b958f', 'disabled' ], | ||
| [ 'b3247f75-fd94-4fef-97f9-5bfd162cc416', 'disabled' ], | ||
| [ 'e178812d-ce5e-48c7-a945-8ae4ffcbbb7c', 'disabled' ], | ||
| ] ), |
There was a problem hiding this comment.
Now we have to list out all the blocks that inherit the disabled mode from the parent so that's mostly what the test changes are.
| @@ -433,7 +473,7 @@ describe( 'private selectors', () => { | |||
| ], | |||
| [ | |||
| 'e178812d-ce5e-48c7-a945-8ae4ffcbbb7c', | |||
| '9b9c5c3f-2e46-4f02-9e14-9fe9515b958f', | |||
| 'ef45d5fd-5234-4fd5-ac4f-c3736c7f9337', | |||
There was a problem hiding this comment.
This test had some mistakes in it. We can see from order that the parent of 'e178812d-ce5e-48c7-a945-8ae4ffcbbb7c' is 'ef45d5fd-5234-4fd5-ac4f-c3736c7f9337'.
It meant changing a bunch of stuff about the test.
| @@ -12,11 +12,13 @@ | |||
| }, | |||
| "isLink": { | |||
| "type": "boolean", | |||
| "default": false | |||
| "default": false, | |||
| "role": "content" | |||
There was a problem hiding this comment.
I think I might move these changes to the post blocks to another PR, as they're really not relevant to the topic of the PR.
| if ( ! isNavigationMode ) { | ||
| for ( const clientId of templateParts ) { | ||
| setBlockEditingMode( clientId, 'contentOnly' ); | ||
| } |
There was a problem hiding this comment.
When editing a page (with 'Show template' on) in write mode, template parts are now 'disabled' (because they're not set to 'contentOnly').
| const updatedBlock = getBlockTreeBlock( | ||
| nextState, |
There was a problem hiding this comment.
The change here is because SET_BLOCK_EDITING_MODE is often called on the root block (with a client id of ''). When you do nextState.blocks.tree.get( '' ), it returns an object that doesn't have a clientId property and that causes some of the later code to fail. getBlockTreeBlock works around that by patching the returned object to have a clientId.
It might be better to make sure the root in the tree does have a clientId property. 😄
To expand on this, for the site editor the 'first block' test is showing a drop of around 30-40%. This is a test that edits a page in the site editor, so the changes to how disabled blocks work in this PR is going to be the problem. I'd expect this if there are lots of actions being triggered on editor load that cause the derived block editing modes to be recalculated, which there are.
Also, the way that component finds template part client ids involves looking through every single block, and it'll recalculate whenever blocks are inserted/removed. A hack for this could be to find template parts from the list of controlled blocks which is shorter and less likely to change. Block syncing for template parts also seems to trigger a lot of this. It replaces inner blocks for template parts multiple times, giving them new clientIds in the process. Some of that might be down to the way react triggers effects multiple times in dev mode. |
|
Think I've solved the performance issues now, it mostly seems to have come down to some small changes to |
|
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @xavier-lc. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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. |
I've noticed this testing on related work. It's really quite bad and we should consider revising it asap. |
|
|
||
| // Template parts become sections in navigation mode. | ||
| const _isNavigationMode = isNavigationMode( state ); | ||
| if ( _isNavigationMode && blockName === 'core/template-part' ) { |
There was a problem hiding this comment.
Why do template parts and pattern have different behaviors? Should they be "sections" always?
There was a problem hiding this comment.
Yeah, I'd love to align this more. It mostly comes down to the way synced patterns and blocks with contentOnly template lock are locked from being edited all the time.
Template Parts are still fully editable when editing a template, so they only act as these contentOnly sections in write mode.
This could be aligned more, but I think there's also a UX part to this.
| for ( const clientId of disabledIds ) { | ||
| unsetBlockEditingMode( clientId ); | ||
| } | ||
| } ); | ||
| }; | ||
| }, [ templateParts, contentOnlyIds, disabledIds, registry ] ); | ||
| }, [ disabledIds, registry ] ); | ||
|
|
||
| return null; | ||
| } |
There was a problem hiding this comment.
What would it take to move away from this component entirely? The code here is so confusing.
What happens if we passed a list of block types as settings to the block editor: editableBlockTypes: [ 'core/post-content', 'core/post-title' ] or something like that, would that be a valid solution?
There was a problem hiding this comment.
Yeah, it is confusing! There's already a list like this - https://github.com/WordPress/gutenberg/blob/trunk/packages/editor/src/components/provider/use-post-content-blocks.js
I think your idea works for most of the blocks, apart from the template part where the block itself is enabled but inner blocks are disabled. This is buggy anyway, as disabling the inner blocks like that doesn't prevent insertions, so at the moment you can still drag things into the template part block when editing a page. 😓
I personally think it'd be best to remove the way template parts are selectable in this mode. There's a parallel discussion on it here - #67621 (comment).
There was a problem hiding this comment.
Another thing is that in the long run, I think it'd be interesting to determine these blocks via their bindings.
E.g. if the user is editing a page with the id 12, then any blocks with bindings to that entity become editable.
That's a bit far away, but it makes me thing that rather than editableBlockTypes we should instead pass editableClientIds as the editor setting.
Blocks with a post meta binding could already be made to work like this if they're in the template.
There was a problem hiding this comment.
I think all these exceptions we're introducing to please some specific UX affordance are really not helping. We keep constructing and deconstructing. I'm not sure how to solve it personally but I think we should consider scaling back UX wise maybe and trying to systemize these changes.
It's really not clear to me why we're treating template parts differently than reusable blocks. We should (at least in our minds) consider these two things the same.
|
Noting that with write mode, we often have to click to highlight which blocks to edit and double clicking then leads to the isolated editing mode. More than a few times, I landed there by accident! I think in Write mode, we need to turn off isolated editing in general, both with the "edit" option in the toolbar and the double click to edit. |
What?
Fixes #66463
Allows editing template parts in Write Mode. Also makes blocks like Site Title and Site Logo editable.
How?
Adjusts the block editing mode rules for Write Mode to make Template Parts that are outside the Section Root
contentOnly.Also makes content blocks within a template part
contentOnly.Then finally updates a range of blocks to be considered
contentblocks by adding"role": "content"to any suitable attributes.Problems to solve
I think the PR is almost there, but in my own testing:
"role":"content". (another follow-up issue)Testing Instructions
(I find it's best to use Twenty Twenty Four for testing, as that theme has lots of sections out of the box)
Template editing
Page editing
Screenshots or screencast
Kapture.2024-11-29.at.12.38.36.mp4