-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
First pass at adding simple editing mode #65027
Conversation
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
7ba2263
to
32021a3
Compare
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. |
61eb31f
to
df42492
Compare
packages/editor/src/components/simple-editing-mode-selector/index.js
Outdated
Show resolved
Hide resolved
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change is incorrect. I've been looking at how to amend it so that:
- you see the
Content
panel [of the nearest "Section"] in the inspector controls. Similar to when settingtemplateLock: contentOnly
on a Group block. - you see the Inspector Controls (contentOnly version) for the selected block
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Drill down might work
as={ | ||
window.__experimentalSimpleEditingMode | ||
? SimpleEditingModeSelector | ||
: ToolSelector |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
edit
to represent what we want to call 'design'. Not unsolvable, but definitely awkward. We might want to deprecate the current values and go for something likedesign
andcontent
as the values. - From a user perspective, the editing mode seems like it can work alongside
zoom-out
mode, butzoom-out
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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=content
attribute setting in block json)
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.
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 mountDisableNonPageContentBlocks
if you're editing a page – same as before. But if you're editing a template then we mount a newContentOnlyLockSections
component.ContentOnlyLockSections
achieves the equivalent of settingtemplateLock = 'contentOnly'
on every section. A section is a child of the block denoted by thesectionRootClientId
editor setting. We useuseBlockEditingMode
to 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:
useBlockEditingMode
is an awful API for this use case! Ideally we'd have a way to settemplateLock
on each section without modifying any block attributes. I've ranted to @talldan before about ditching this API and possibly evolving ourlock
andtemplateLock
APIs 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.