Add: Content lock ability. #43037
Add: Content lock ability. #43037
Conversation
jorgefilipecosta
added
the
[Feature] Patterns
|
Nice, this is working fairly well. A couple of observations: We need a way to handle non-text content like images. In the example above it should be possible to swap the image source. It's a bit strange to find the parent container in the 'content' section of the Inspector, since that is not really 'content'. We'll need to adjust the design a bit there. There's something a little bit awkward about clicking the block in the Inspector and being taken to the settings panel. I think it's because it looks like list view, and so I've come to expect that clicking will just select the block. Having both actions take place simultaneously (select & reveal settings) feels a bit unexpected. Perhaps we need a dedicated 'Settings' button or something. A more high-level thought that may impact some of the above: 'content lock' kind of suggests to me that only content can be edited. Consequently this brings into question whether it should be possible to adjust the style of the content blocks inside the container? It would simplify things a great deal if you were only able to edit the actual content (and use the tools in the toolbar) here. If it's not a lot of work, it could be interesting to try a version of this where you cannot access the settings panels. |
|
Nice, this is a potent PR, let's be sure to land a variant of this in time for 6.1. I took it for a quick spin: I'm still absorbing the feature, so I'll need to think more about this. But a few very quick first impressions:
Something that I'll need to think more about is the heuristic for which blocks show up as still selectable, and which ones don't. I have to edit as blocks to change the image, but I can change the heading level. Mainly this feels a bit confusing as I can clearly see there's an image present, but it doesn't show up in the list view. Part of me would like for all of this to be locked down even further, especially for the first iteration. As an unformed thought is to not show any child blocks in list view when locked, and to use a drastically simplified block toolbar for any editable fields inside. Something closer to the caption toolbar we have: I.e. perhaps instead of this:
maybe something like this:
I'll come back with more thoughts, but wanted to share immediate reactions. Nice work! |
| if ( getBlockAttributes( state, rootClientId ).lock?.content === true ) { | ||
| return 'all'; | ||
| } | ||
|
|
There was a problem hiding this comment.
Looking at this change, I wonder if it makes sense for content locking to be a part of templateLock, instead of block locking.
There was a problem hiding this comment.
Good question 👍 Yes, I guess we should consider making it a templateLock = content. It seems to make sense.
There was a problem hiding this comment.
I've been educating myself about the different locking mechanism we have. I agree that using templateLock makes more sense than lock because templateLock aims to control the inner blocks behavior while lock controls the block itself.
I see some challenges:
- Naming. We should follow the existing naming schema and indicating what it locks, not what it allows to edit.
noContentcould work. - Semantics. Introducing a new state to
templateLockthat is not absorved by the existingallis going to create disonance for users of the API. Example:- inserts => locks insertion/removal
- all => "inserts" plus locks movement
- noContent => "all" plus a bunch of other things (prevent selection for blocks without content, hide them listview, absorb the block inspector of the inner blocks, etc)
- Nested templateLock. The existing locking mechanism fallbacks through a hierarchy: PostType's templateLock < an InnerBlocks's templateLock < another InnerBlocks's templateLock < a singe Block's lock. The latest has more priority. Does the new
noContentstate also flows through the hierarchy? If so, what's the expected behavior here:
<group templateLock=noContent>
<group templateLock=all />
</group>- Following the reasoning above, there's an existing
templateLock=falsestate. If a block has this state, it "unlocks" anything defined up in the hierarchy. Some blocks use it: social links, navigation, and column. If we respect the cascading behaviour andfalsecan unlock anything, the new state won't be effective for those blocks. templateLockandlockand interrelated. We should consider introducing the corresponding states forlockif we introduce one fortemplateLock.
Chewing about it, this is the next steps I see:
- Update this PR to use a new state in templateLock instead of lock. Despite the challenges it makes more sense.
- Ignore the value of templateLock for nested containers in this PR. For controlling scope, it should be addressed in a follow-up.
- We may need to consider exposing
templateLockas an attribute of colum, navigation, and social links should thefalsestate needs to be overriden by a pattern. To be tested and investigated when addressing nested content. - I'm unsure about introducing a new state for lock. It makes sense conceptually, though I can't see now any practical uses for it. Punt it for the future if need be.
Thoughts?
There was a problem hiding this comment.
Pushed changes to substitute lock.content by templateLock: "noContent". It should work the same as before.
I think this is worth a try, possibly in conjunction with not providing access to the inspector for child blocks as well. These feel somewhat linked to me, and fit the notion of shipping something sooner that is more lightweight. It would solve the somewhat awkward duplication of child blocks appearing both in list view and the Inspector. And the interaction quirk I pointed out in my last comment, where clicking the child block's name in the Inspector has two detached actions (select + expose settings). It would probably feel more natural if it was just a selection tool.
That's a good thought. There's really no need to display the block type in the toolbar, especially as transforms shouldn't be possible in this context. Ditto the lock icon – the lock status of these blocks should probably be governed inherently by the parent's 'content lock'. Just to connect a dot, could we somehow leverage the work in #42399 to achieve this? I would add that it might be worth keeping the parent selector though. It will be helpful in configurations where child blocks fill the height/width of the parent. |
I had that thought too. It might, but given how trivial it was in the inspector to hide a few of the toolbar segments, it might be okay to do a temporary thing in the mean time?
I don't feel strongly about this. But I do kind of feel like having to select a layer kind of goes against the main purpose of "flattening" a pattern like this. What might be a use case where one such thing was needed in this flattened hierarchy? And might there be a different way to accomplish it? A transversal thought: at the moment a block has options in both the toolbar and in the inspector. I recall @critterverse thinking a lot about this split back in the day, and it remains a bit messy. As configuration tools in Global Styles → Blocks increasingly get added and mature, it's also becoming clear that if you need to be able to configure a block default, it has to be present in the inspector. It cannot live only in the block toolbar, lest it just won't be fully accessible there. (Because you might be styling a Featured Image on a template that doesn't yet have a featured image inserted in the canvas to select). This could suggest that every action present in the block toolbar must, somehow, also be present in the inspector. The block toolbar would still afford an immediate and intuitive access point, but nevertheless the option has to be present in the inspector. Which means absorbing controls in an interface like this could potentially be even simpler still. Something to think about. |
I don't feel strongly either. It's on my list to test this PR with different block configurations, I think that will help to make a decision. |
| @@ -789,3 +789,13 @@ export const hasChildBlocksWithInserterSupport = ( state, blockName ) => { | |||
| return hasBlockSupport( state, childBlockName, 'inserter', true ); | |||
| } ); | |||
| }; | |||
|
|
|||
| export const __unstableIsContentBlock = createSelector( | |||
There was a problem hiding this comment.
What do you think of expanding the existing naming (example) and use something along the lines of __experimentalHasContentRoleAttribute? As the first time I ran into this, it was not clear what a "content block" was.
| ); | ||
| } | ||
|
|
||
| function BlockNavigatorScreen( { block } ) { |
There was a problem hiding this comment.
What do you think of extracting these components to its own file? The amount of supporting components/utils is high and introduces noise when one wants to understand the high-level flow.
| blockTypes={ blockTypes } | ||
| /> | ||
| ) ) } | ||
| { showSelectedBlock && ( |
There was a problem hiding this comment.
We can actually still select blocks that don't have content. The question is what we do in that situation. This is what happens now:
selected-block.mp4
My first instinct is that this interaction is at odds with what this feature wants to achieve (hide blocks with no content). So, what do you think of showing the top-level block inspector controls when this happens?
| ); | ||
| } | ||
|
|
||
| function BlockInspectorAbsorvedBy( { absorvedBy } ) { |
There was a problem hiding this comment.
I've had troubles to parse what BlockInspectorAbsorvedBy and absorvedBy meant. I'm not sure about the right naming here and wanted to share some ideas to see if we can come up with a better naming schema: BlockInspectorLockedBlocks and topLevelLockedBlock?
|
Hey, I've reviewed parts of this code (listview, blockinspector) and need to finish it. In the meantime, I have a conceptual doubt. I've read some of the linked issues and the relationship between this lock and the "block lock" mechanism we have in place is not clear to me. What's the difference between:
And this other lock:
In principle, it sounds like the work here could go into the "lock modal" at a visual level. Conceptually, the lock in this PR feels like a "special/smart" lock that does a few things beyond what the other locks do (affects the inspector and listview) though fall into the boat. Does that make sense? It'd be helpful to finalize the technical review to have more context about these things. |
The other locks allow restrictions on the block like movement or removal, and that's it. This lock is something more advanced. It not only applies restrictions of removal and movement but also tries to make things nested in an area look like a single block, where only the blocks with content are editable. |
|
Hi @jameskoster, @jasmussen, @oandregal, thank you for the reviews.
Images are going to be content when #43038 is merged.
I would say not the parent because we don't want a hierarchy concept, but on all the toolbars the top block that has the lock would appear as the parent, so we always have a way to select it even if children take up all the space. It would appear as if all the content blocks are direct children no matter what other blocks are in the middle. So here are the things we should try/next steps.
Does this list of next experiments seems correct, or is there anything I'm missing/should not be tested now? |
|
Sounds good to me.
Just to clarify, it would be the exact same categories we remove across all inner blocks, it would not be a curated list. Specifically, it would be the "mover group" and the ellipsis, always. So in the case of paragraphs and images, it would be these sections: |
|
I've given this a try using the blockquote that can have innerblocks and also the cite which is a richtext field. Note how:
I'll have a look at what can we do here in the context of Nik's comment about how the nuances of |
I gave this a shot and it looks like we need to live with this for now. The issue is that the quote's cite is not a block so it's only editable if we make the quote itself editable. If we remove the "role=content" from the quote atts (see), there's no way for the user to edit the cite. |
oandregal
force-pushed
the
add/content-lock-ability
branch
from
333bd54
to
0aeb4aa
Compare
|
Size Change: +1.73 kB (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
|
@oandregal @jorgefilipecosta it confused me a lot to see the value being |
|
Yeah, that was my concern. I've prepared #44131 to update |
fabiankaegy
added
the
Needs Dev Note
|
I've just added the Needs Dev Note Label as I think this will be very impactful in WordPress 6.1 :) CC: @bph |
|
Just to be clear for documentation purposes (as I think the PR name may be a bit misleading) but this mechanism locks everything but content blocks, yea? And should we refer to this as "Content-only locking"? Not too thrilled with that term myself. |
|
It locks child block position / layout, addition, and removal. Text and attributes like image source remain editable. The objective was to make complex groups feel more like single blocks. "Content-only editing" might be better? "Group template" could be an option, but "template" is obviously a loaded word already. "Flattened groups"...? Naming is hard. |
|
#44521 makes the new experimental/unstable selectors introduced in this PR private. |
|
Out of curiosity, is there a mechanism to limit which user roles have the ability to "Modify" the actual blocks?
I tried to look through the code to see whether it was possible but it didn't seem to be the case. I think it would be a great addition to this feature to be able to control which user role levels have the ability to modify the actual block structure / vs which roles can only edit the content :) Thanks in advance for any feedback :) |
Building off of the launch of this new functionality in 6.1, this updates the curation resource to include information about content lock ability: #43037
|
Created #45263 as a small followup that should be easy. |
* Updating curation document to include content lock ability Building off of the launch of this new functionality in 6.1, this updates the curation resource to include information about content lock ability: #43037 * Slight update to the children blocks language * Update to use "content only editing" and clarify block usage
As referred to previously in a core editor chat, for now, we did not implement role-based restrictions, but it is something we can consider in the future. Thank you for the suggestion 👍 |
As shared by @jameskoster in #42684 (comment) we are exploring a content lock with the following requirements when a container is 'content locked':
Screenshots
Sample
Testing
Missing
UI to add a content block. For now, we can hardcode on the code editor. But a UI could be offered either as part of the lock modal or as part of a UI to create patterns on the editor.