-
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
Add ability to set templateLock = 'contentOnly' in editor settings #50082
Conversation
if ( selector.isRegistrySelector ) { | ||
selector.registry = registry; | ||
} | ||
|
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 bit of code was missing. It's the same as what's on line 208 above. Without it you get an error when using createRegistrySelector
for a private selectors.
bd6eea4
to
4fc3d10
Compare
Size Change: +158 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
Flaky tests detected in 4fc3d10. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4805724939
|
Thank you for working on this :) I can see this also being a great API to eventually expose the template in the post editor similar to the site editor. (#27847) |
So if I'm understanding properly the long term goal of this PR, the idea is to be able to edit the post separately from the template while still rendered within the template. It seems that you arrive to the conclusion that the locking needed for that use-case is the same that we already have to "lock" patterns (containers). Personally, I'm still wrapping my head around this assumption. Can you explain more why it's the same thing for you? |
Yes absolutely @youknowriad! The goal of #49980 is to improve the UX of editing page content within the site editor. You can already do this today but it's overwhelming to many users (especially new ones) because it requires knowing which blocks are a part of the template versus which blocks are a part of the page. This results in e.g. accidentally making changes to the entire site when the user just wanted to modify one page. The hope is that by locking the template blocks it will be less overwhelming. The goal of this PR is to add locking functionality to So our requirements for the locking mechanism are:
It turns out that the existing
We can address (1) by fixing the buggy behaviour and we can address (2) by adding an explicit way for I favour this approach because:
The one thing I am not so keen on is the new I'll cut myself off here 😀 Let me know what you like, don't like, and if you have suggestions. |
This was also the thing that stood out to me when reading the PR description. I'm also not sure about extending Another alternative is that |
On the The problem with the former is it doesn't work for Post Title and Post Featured Image which don't use The problem with the latter is that it creates an undo level and marks the template as having unsaved changes. We would need a way to set |
Something I'd like to clarify, is the goal for post content to be completely editable (add/remove/modify blocks?), or should it still be content locked in some way?
Yeah, this is an issue that has come up before with those blocks, and one of my concerns about It feels like two different problems to me. One is that |
Yes. |
I suppose it depends really. If it's the editor initiating a mode, then it probably shouldn't be saved to block attributes, else you get issues like #49048. There is already an |
This action is unfortunately only really an |
That's a shame, but it also makes sense now you say it. 😄 I guess you need another way. |
Just to be clear, I actually like the fact that this reuses an existing API instead of inventing one. I'm just trying to make sure we're not breaking the API or that the API is actually meant for that.
I guess my first question here was that: are we talking about the same "content"? In other words, what does "content" really mean?
I guess I'm looking at a clear definition of "content": content attributes (we have) and content blocks (less clear to me)
Did you consider useDisabled (and potentially merging with the behavior we already have in
When the whole editor is "content only", does it mean all blocks are in "content only", or all blocks but the content blocks. If the "content blocks" reset the template lock for their children, does this behavior has any impact on locked patterns? |
👍 absolutely.
"Content block" within the context of this PR refers to a block that is not affected by a
I'll double check but I think we may already get this for free because when there is a
The latter. The toolbar buttons etc. are hidden if the block is "content locked". A block that is a descendent of a "content block" cannot be "content locked". I've tested locked patterns and all is fine, but admittedly I haven't tested putting a locked pattern inside a content locked locked editor which probably has issues to do with |
If I follow this definition, it means I should be able to edit the content of a paragraph within a template even if contentOnly mode because the paragraph block has an attribute with role "content". I'm assuming that's not the case and we basically have two definition of "content blocks"
maybe I'm wrong though?
I haven't tested either but it feels that the behavior of a "locked pattern" that I insert within post content, shouldn't be impacted by the editor being in "contentOnly" mode. |
Yes you have it right. If
Yeah. It should be possible to insert a locked pattern into the post content, but this is probably broken. I am convinced now that |
I guess the two directions I can take this are:
Wdyt? |
I'm starting to feel like "contentOnly" locking is something else and that we're stretching that concept a bit too much here. What if instead of "locking" specific parts of the editor, we think about this problem in the opposite direction: we want to "enable" specific parts of the editor.
So what about something like that: 1- block-editor exposes a Pros of this approach: No impact on third-party blocks, no new public APIs |
I guess this is very close to @talldan's proposal as well. (but I don't see this as "template locking" personally, it's more "disable editing entirely" thing. |
Something that appeals to me about re-using I like your idea, I'll try it out tomorrow. The "Post Title can decide to allow editing anyway" problem can maybe be solved by having Thanks for your help @youknowriad! |
Indeed, I guess we need to figure out whether third-party blocks are meant to implement this or not. |
I started playing with the idea of using context in this branch if you want to take a look: trunk...try/locking-v2 I don't think we can use I haven't yet figured out how to handle hiding the parent block selector, block movers, block toolbar buttons, in-between inserter, etc. It's tricky because these components live outside of the canvas and so don't have access to the context. Will keep fiddling. Something else I want to explore is if we can make |
There might be a situation where a user also wants to edit Site Logo / Site Title in content only mode. It's been identified that patterns could use content only mode (see 'The wp:pattern block'), and they are useful site building blocks (particularly headers and footers). If those two things co-exist then potentially a user will want to upload a site logo once they've chosen a header pattern and not have that block completely locked down when content only mode is active. There's still a long way to go in exploring these things, but just saying there could be some overlap with what you're looking at with 'post' blocks. |
I'm not clear why the user should be able to do these things in "content only" mode? Maybe it's the name "content only" that is confusing, but for me what we're trying to do here is not "content only mode" but "entity only mode" or something (editing a post vs editing everything). Now yeah, there's a question about blocks used within templates and that store things outside the templates (site logo, site title, navigation block, reusable block). What should we do about these? It's unclear to me personally and I'd love some thoughts from @jameskoster and @SaxonF but my instinct is to say that if the blocks are within the template prevent editing them, if they are within the post content block, allow editing them. But I agree that we need clear rules and stick by them. |
Perhaps it's helpful to remind about the scenario where a user should encounter this 'mode'. In the site editor pathways are emerging that enable users to edit their pages. In such flows it's currently way too easy to mistakenly add blocks to the template, believing you're editing a page. So it becomes important for us to prioritise the direct manipulation of any data associated with the page, and de-prioritise everything else. Generally speaking that means we want the In this sense I'd agree that "entity only" might be a better way to describe it. To begin with we'll need to support single posts and pages as entity sources, but it's also possible that the entity could be something like a post category. |
Will close this out. I've a new approach in #50643. |
What?
Editor content locking
In #43037, the ability to content lock blocks was added. This is done by setting the
templateLock
attribute on any container block (e.g. Group) to'contentOnly'
. This is useful for creating patterns where the user may only edit content and not add, remove, or edit blocks. See https://richtabor.com/content-only-editing/ for a great guide on doing this.This PR expands this functionality to work at the editor level. You can now set
templateLock
to'contentOnly'
in the block editor settings. Doing this results in the exact same experience as above, but applied to all blocks within the editor.Kapture.2023-04-26.at.13.56.02.mp4
This PR also adds the ability to explicitly state which block types are considered to be "content" via the
contentBlockTypes
editor setting. This gives one the ability of finely control the locking. For example, to lock all editing except for the content within a Post Content block, one can specify:Because container blocks (e.g. Post Content) can now be designated as content, the content locking functionality has been added to correctly take into account block nesting. For example it is possible to move and delete blocks within a content block.
List View changes
Currently, when a container block has
templateLock = 'contentOnly'
, the List View hides all of that container block's children. This behaviour no longer makes sense now that the editor can havetemplateLock = 'contentOnly'
as this would result in a completely empty List View. Therefore this PR adjusts the List View to display only the content blocks that are nested within a locked container.Block inspector changes
The list of content blocks that appears in the block inspector when a locked block is selected has been updated to use a
Panel
component for consistency and to re-use the same List View logic.API changes
The content locking APIs (selectors and actions) have been rewritten for clarity around three defined terms: content locking block, content locked block, and content block.
isContentLockedBlock
(formerly__unstableGetContentLockingParent
) has been marked stable as it is the primary way that blocks adjust their interface in response to a lock. All other selectors and APIs have been marked private.In addition there is a new
isInsertionLocked
selector which is similar in nature tocanRemoveBlock
,canMoveBlock
, andcanEditBlock
. Having this lets us avoid repeating logic that checksgetTemplateLock
over and over.isInsertionLocked( rootClientId )
(public)__unstableGetContentLockingParent( clientId )
→isContentLockedBlock( clientId )
(public)__unstableGetTemporarilyEditingAsBlocks()
→getTemporarilyUnlockedBlock()
(private)__unstableSetTemporarilyEditingAsBlocks()
→setTemporarilyUnlockedBlock()
(private)getContentClientIdsTree( clientId )
(private)isContentBlock( clientId )
(private)getContentLockingBlock( clientId )
(private)isContentLockingBlock( clientId )
(private)Why?
This forms the basis for how #49980 is implemented.
Testing Instructions
To test the new functionality:
wp.data.dispatch( 'core/block-editor' ).updateSettings( { templateLock: 'contentOnly' } )
.To test the existing functionality for regressions: