First pass at adding simple editing mode #65027
Conversation
noisysocks
added
[Type] Technical Prototype
|
Size Change: +1.8 kB (+0.1%) Total Size: 1.78 MB
ℹ️ View Unchanged
|
packages/editor/src/components/provider/content-only-lock-sections.js
Outdated
Show resolved
Hide resolved
| for ( const clientId of sectionClientIds ) { | ||
| setBlockEditingMode( clientId, 'contentOnly' ); | ||
| } | ||
| for ( const clientId of sectionChildrenClientIds ) { | ||
| setBlockEditingMode( clientId, 'disabled' ); | ||
| } | ||
| for ( const clientId of contentClientIds ) { | ||
| setBlockEditingMode( clientId, 'contentOnly' ); | ||
| } |
There was a problem hiding this comment.
I recognise this pattern is necessary but - as you've noted - it might be an indicator that we need to design a new API for this use case.
However, I fully support a "get it working" approach here.
| const allClientIds = sectionRootClientId | ||
| ? getClientIdsOfDescendants( sectionRootClientId ) | ||
| : getClientIdsWithDescendants(); |
There was a problem hiding this comment.
This code is appearing in various PRs. I think we should have a selector which finds the "sections". I don't think it's a premature abstraction because it will make the code easier to reason about.
getdave
force-pushed
the
try/simple-editing-mode-selector
branch
from
7ba2263
to
32021a3
Compare
|
When I enable the experiment, I'm not seeing the control consistently. Is it because of the top-level selection? I think it's working on template editing, but not pages.
|
|
I've opened an alternative PR #65055 that tries to approach this same problem but from a different angle. |
I would test on templates for now. I've been testing again TT4 home with no modifications. Also be sure to enable the "Simple mode" experiment. |
Yes Simple / Advanced is hidden on pages because on pages we already have several |
Having not tested any of this simple / advanced mode stuff here this note worries me a little bit. In 6.6 we introduced a filter to allow extenders to modify the list of "Post Content" blocks that are still accessible / editable when rendered in this That is the foundation we are using for custom builds these days. I'm all in favor of further simplifying and improving these experiences and love the direction all of this is taking. But I want to make sure we stay somewhat backward compatible and don't remove abilities we released in the public |
|
Yeah, those are public APIs, so "removing" that feature would be more about hiding it from the UI rather than deleting code. Early days though 😀 this is all ideation and exploration at this point. |
getdave
force-pushed
the
try/simple-editing-mode-selector
branch
from
61eb31f
to
df42492
Compare
packages/editor/src/components/simple-editing-mode-selector/index.js
Outdated
Show resolved
Hide resolved
|
Using DropdownMenuV2
|
|
One thing we should try is to add back the "dark" block toolbar styling that provided a visual change previously with "select" mode. This would help communicate the "mode" better. Current
Proposed
|
|
We need to narrow down the exact phrasing for these tools, even if we are still iterating on them:
We need to write down some issues for how to control what mode is a default, whether it persists, how to assign a default role / capability to them, how to restrict based on role, and so on. |
|
@youknowriad work on #65055 is relevant for the toolbar change of appearance. We should consolidate the concepts a little bit in their structure. |
|
I've been a bit time poor (kid is sick) but quickly sharing some things I'd like to try in this PR:
We also need to sort out the best approach re. Then, not for this PR, but some related things I'd like to try that might form together to create a nice "page building flow":
|
Get well soon 🙏
Just noting that some of this probably heavily overlaps with Zoom Out mode. |
| @@ -117,7 +128,8 @@ const BlockInspector = ( { showNoBlockSelectedMessage = true } ) => { | |||
| ( getTemplateLock( _selectedBlockClientId ) === 'contentOnly' || | |||
| _selectedBlockName === 'core/block' | |||
| ? _selectedBlockClientId | |||
| : undefined ), | |||
| : undefined ) || | |||
| closestContentOnlySectionBlock, | |||
There was a problem hiding this comment.
My understanding of the changes in this file is that we want to show the "inspector control" of the parent "content-only" block when it exists rather than the leaf block that is being selected.
Are we doing the same for the block toolbar. This also feels like something we want to do consistently for block inspector and block toolbar?
There was a problem hiding this comment.
I think this change can probably be extracted to a dedicated PR as it can be seen as an improvement to content only blocks. would be good to e2e test as well.
There was a problem hiding this comment.
That change is incorrect. I've been looking at how to amend it so that:
- you see the
Contentpanel [of the nearest "Section"] in the inspector controls. Similar to when settingtemplateLock: contentOnlyon a Group block. - you see the Inspector Controls (contentOnly version) for the selected block
There was a problem hiding this comment.
you see the Content panel [of the nearest "Section"] in the inspector controls. Similar to when setting templateLock: contentOnly on a Group block.
you see the Inspector Controls (contentOnly version) for the selected block
If we want to combine two blocks in the inspector controls, I feel we need some design explorations no? Or do we just dump everything there? It maybe hard for users to understand what's happening no?
There was a problem hiding this comment.
@richtabor What are your thoughts here? If you select a child block of a section, should we be exposing the currently selected block's inspector controls or just the parent "section" block's controls?
If it's the former how can we combine that with the Content panel being shown at all times without causing overwhelm?
There was a problem hiding this comment.
My understanding of the changes in this file is that we want to show the "inspector control" of the parent "content-only" block when it exists rather than the leaf block that is being selected.
Are we doing the same for the block toolbar. This also feels like something we want to do consistently for block inspector and block toolbar?
I think this is already how contentOnly template locking works, as does the pattern block.
It's something that's always felt a little off to me (having a different selected block shown at the top of the inspector compared to the block toolbar), but I think it might be worth breaking it out into a separate issue to discuss given the precedence.
There was a problem hiding this comment.
I think we should be consistent with what happens when you have a contentOnly pattern for now.
Wouldn't mind changing it in a separate PR though. Either to use a drill-down UI like we do for the Navigation block or to just get rid of it entirely.
| as={ | ||
| window.__experimentalSimpleEditingMode | ||
| ? SimpleEditingModeSelector | ||
| : ToolSelector |
There was a problem hiding this comment.
I'm not sure why we're using a separate tool selector personally, can't we reuse the same?
There was a problem hiding this comment.
I think this was just for the purposes of getting an experimnet up without editing the other component. We should be able to reuse the ToolSelector - especially if we use Select mode.
There was a problem hiding this comment.
It had to be a new component so that it could live in packages/editor where rendering mode is (not packages/block-editor where editing mode is) and so that we could switch it when the experiment is enabled/disabled. If we're okay with using editor mode (see other thread) then yeah we can re-use the existing component.
| [] | ||
| ); | ||
|
|
||
| const { setRenderingMode } = useDispatch( editorStore ); |
There was a problem hiding this comment.
I don't understand why we're using the "rendering mode" instead of the editor mode (navigation mode) for this. These two feel to me like different things and that we shouldn't be touching the rendering modes for this PR.
There was a problem hiding this comment.
I expect it was a quick way to get up and running. There a couple of things that need to be solved with using editor mode:
- Editor mode already uses a value of
editto represent what we want to call 'design'. Not unsolvable, but definitely awkward. We might want to deprecate the current values and go for something likedesignandcontentas the values. - From a user perspective, the editing mode seems like it can work alongside
zoom-outmode, butzoom-outis already an editor mode so with the way it works right now the two couldn't be active at the same time. Seems like we'd need to figure out how we want the two to work together and exactly what the right primitives are for representing these modes.
There was a problem hiding this comment.
I used rendering mode because it's in editorStore not blockEditorStore and I was thinking that this feature isn't really something that all block editors should have. (Maybe it is, though.)
There was a problem hiding this comment.
From a user perspective, the editing mode seems like it can work alongside zoom-out mode
I don't think we should allow this. When zoomed out, the text is small and not conducive to editing.
To me there's three "modes":
- Zoomed out. Focus is on patterns and previewing the whole page
- Edit mode. Focus is on patterns and editing the content
- Design mode. Focus is on blocks
So yeah sounds like editor mode is a better fit 👍
| // 4. ...except for the "content" blocks which should be content-only. | ||
| for ( const clientId of contentClientIds ) { | ||
| setBlockEditingMode( clientId, 'contentOnly' ); | ||
| } |
There was a problem hiding this comment.
Can we start moving away from setBlockEditingMode usage and instead update the "getBlockEditingMode" selector to guess the right mode based on the state instead?
There was a problem hiding this comment.
I see. So rather than batching multiple actions, instead we make the selector more complex so that it can infer the correct block editing mode on a per block basis?
There was a problem hiding this comment.
yes, that makes the code more solid IMO. because it's easy for folks to render competing components like the ContentOnlyLockSections here and you end up with race conditions. For instance, I know we had the navigation block at some point call setBlockEditingMode directly which conflicted with the locking applied based on the "editor package rendering mode" and the more we add this kind of components, the worse it gets.
There was a problem hiding this comment.
I'll take a look at this. I'd also love to solve the similar situation in the pattern block, though I wonder if it'd be ok to build so much into one selector.
There was a problem hiding this comment.
The idea was that folks would use useBlockEditingMode in their blocks but yeah unfortunately it's become more common to call setBlockEditingMode directly which has nasty side effects. Need to revisit this API in favour of something more declarative. Maybe an array of "locks" that is passed in via block settings.
| @@ -37,6 +37,10 @@ function gutenberg_enable_experiments() { | |||
| if ( $gutenberg_experiments && array_key_exists( 'gutenberg-zoom-out-experiment', $gutenberg_experiments ) ) { | |||
| wp_add_inline_script( 'wp-block-editor', 'window.__experimentalEnableZoomOutExperiment = true', 'before' ); | |||
| } | |||
|
|
|||
| if ( $gutenberg_experiments && array_key_exists( 'gutenberg-simple-editing-mode', $gutenberg_experiments ) ) { | |||
| wp_add_inline_script( 'wp-block-editor', 'window.__experimentalSimpleEditingMode = true', 'before' ); | |||
There was a problem hiding this comment.
Personally I don't fell like we need an experiment if we take over the "select mode" and improve it.
There was a problem hiding this comment.
If we take over "Select Mode" do we need blockEditingMode at all, or can we just conditionalise all the block tools (in both locations) so that they
- have the right visual appearance.
- show only the necessary controls (i.e. respect the
role=contentattribute setting in block json)
There was a problem hiding this comment.
I think blockEditingMode probably needs to stay for BC reasons but I think ideally the block adapts to the tool without needing to call setBlockEditingMode explicitly.
|
In case you've been following this PR. You might also want to take a look at this alternative PR #65204 which blends this behavior with the "select tool" with a few other refactorings. |
|
Happy to close this in favour of #65204 since it sounds like editor mode (not rendering mode) is the right API to use for this and that we're okay with removing the Select tool. Thanks for the discussion @youknowriad, @getdave, and @talldan. See you in the other PR 🫡 |
Attempting to build a more production-ready version of #64866. Still pretty exploratory though. I'm opening the PR earlier than I normally would so that @getdave can have a sniff.
This PR adds a new Simple editing mode experiment. When enabled, we swap the Tool dropdown in the editor toolbar for a new dropdown that lets you choose between Advanced and Simple. The design of this comes from @SaxonF's mockup in #60021.
Switching to Simple sets the editor's rendering mode (see
setRenderingMode) to'template-locked'.We currently use
'template-locked'when editing a page in the site editor to indicate that the template blocks should be made non-editable usingDisableNonPageContentBlocks. It doesn't make sense to combine this existing locking affordance combined with the Simple / Advanced affordance, so I'm re-using the existing'template-locked'setting. When the rendering mode is'template-locked', we mountDisableNonPageContentBlocksif you're editing a page – same as before. But if you're editing a template then we mount a newContentOnlyLockSectionscomponent.ContentOnlyLockSectionsachieves the equivalent of settingtemplateLock = 'contentOnly'on every section. A section is a child of the block denoted by thesectionRootClientIdeditor setting. We useuseBlockEditingModeto achieve the locking as this is currently the only way to lock blocks without making changes to block attributes which will create undo levels and end up inpost_content.That brings us to the part I hate about this PR:
useBlockEditingModeis an awful API for this use case! Ideally we'd have a way to settemplateLockon each section without modifying any block attributes. I've ranted to @talldan before about ditching this API and possibly evolving ourlockandtemplateLockAPIs into something more general purpose. Maybe it's time.To be continued 🙂
To test: Create a new blank template in the site editor, add some patterns using the inserter, then use the new dropdown in the toolbar to switch to Simple.