Block Locking: Add the 'Apply to inner blocks' option #41876
Conversation
|
Size Change: +216 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
|
|
||
| return { | ||
| isReusable: isReusableBlock( getBlockType( blockName ) ), | ||
| isReusable: isReusableBlock( blockType ), | ||
| hasTemplateLock: !! blockType?.attributes?.templateLock, |
There was a problem hiding this comment.
I'm testing block supports by checking if the templateLock attribute is registered for the block type. We can also convert this into a new supports flag.
There was a problem hiding this comment.
This way is correct, even if we have a supports flag, some blocks may prefer to register the attribute themselves so they can provide a default value to the attribute.
Mamaduka
force-pushed
the
try/container-lock
branch
from
0d19ad6
to
9aca01f
Compare
Mamaduka
requested review from
jameskoster,
mtias,
critterverse,
senadir and
ntsekouras
and removed request for
ellatrix
Mamaduka
added
[Type] Enhancement
|
Thanks for putting this together! Curious what @jorgefilipecosta thinks of using the API this way. |
|
Thanks for exploring this George! I actually started testing without reading the I don't have much context with the locking functionality/code but would be too complex to implement without these limitations? Maybe for example checking innerBlocks.. Sorry if my question is naive due to my lack of context here.. |
| export default function BlockLockModal( { clientId, onClose } ) { | ||
| const [ applyTemplateLock, setApplyTemplateLock ] = useState( false ); |
There was a problem hiding this comment.
By using state here, this is always false when opening the modal.
Screen.Recording.2022-06-23.at.3.51.22.PM.mov
There was a problem hiding this comment.
I thought about defaulting to the templateLock attribute value if it's truthy but reverted it last minute. Happy to re-implement it.
There was a problem hiding this comment.
Having this option off by default plays nicely with recent changes - #41876 (comment).
What do you think, Nik?
There was a problem hiding this comment.
I think we need to reflect the setting applied to the initial state otherwise the users wonder why the checkbox becomes unchecked.
|
Thanks for testing, @ntsekouras.
I think we'll have to add |
That was what I had in mind. We already add the |
This one is a bit unfortunate, it makes this flow feel broken: lock.mp4It's especially strange because if I elect to lock both movement and removal and apply that to children, the movement locking seems to work just fine. If we can't make this work, it might be better to conditionally hide the "Apply the children" toggle based on the selected lock options. |
|
@jameskoster, that's a
|
|
Is it within scope to add more |
|
I think in this PR we can try to disable the toggle button if only the "disable movement" is selected. Moving forwards I think we should try to make the assumption that templateLock exists as a property of any innerblock unless explicitly set to false by a block author. And to align support for the same set of locking tools. |
Agree with @jameskoster and @mtias that it would be nice to only have the toggle available when needed. I mentioned a similar idea of hiding and showing the "apply to children" toggle contextually in this comment: #40069 (comment) For example, maybe the toggle is not visible by default but it appears when you've selected a relevant option ("lock all" or "prevent removal"). |
|
I'd try just setting it to disabled instead of removing so we avoid some jumps as you toggle, but that's more of a detail, especially if we end up adding the missing option in template lock anyways. |
Do you mean we should check for the |
|
@Mamaduka yes, essentially make template locking a default feature of inner-blocks that can be programatically set at any point for any nested group, unless the block implementor excludes it on purpose. |
Mamaduka
force-pushed
the
try/container-lock
branch
from
30ace3c
to
0af0772
Compare
|
I've updated logic to disable toggle when a block cannot apply template lock. @mtias, let's handle that in follow-up PR(s) alongside registering Screenshot
|
|
"Apply to all children" - not many users might realize that means all the enclosed blocks inside the container. Can we come up with a better label for this toggle switch? If that's not possible maybe something like: |
Mamaduka
added
the
Needs Copy Review
I'm not sure if including the block name is necessary or not, but I like "Apply to all blocks inside" quite a bit! |
|
Let's go with "Apply to all blocks inside", the name of the block is already on the title of the modal. |
|
I have updated the label. I think this is ready for final review. |
Mamaduka
removed
the
Needs Copy Review
|
Thank you, @jorgefilipecosta. That's an easy enough fix, and I will push an update shortly. Any concerns about using |
There was a problem hiding this comment.
I think if we have a templateLock in a block we should not render the locking UI in the descendent blocks. Otherwise templateLock may be used in a block that expected its children to be totally fixed because of business constraints and now the user can just go to a child and unlock it. It may break existing blocks that would expect a user to never be able to do that easily.
|
I think templateLock is the right API for this feature.
This way of implementing the feature would be very fragile. I may apply looking at all the children but then I would be able to insert new blocks which seems unexpected when there is total locking in place and these new blocks would be totally unblocked which is also a little unexpected. Regarding the current limitation of just applying to remove or move locking to descendants. I tunk it is not a blocker for this feature. But if we want to address it I see two options:
|
When adding the templateLock attribute for the first time the first approach was a general one that supported every block. Meanwhile, we opted for a more restricted approach where the attribute is only present in some blocks. I can try to revive the old PR/branch, where a templateLock attribute is an option on all blocks. |
Mamaduka
force-pushed
the
try/container-lock
branch
from
adf8f14
to
8262739
Compare
|
Thanks for the review, @jorgefilipecosta 🙇 I've updated the logic to use the
I personally like the idea that inner blocks can be controlled individually. We also have different mechanisms to hide the locking UI on a block or global level. |
What?
Resolves #40069.
PR adds the new "Apply to inner blocks" option to the Block Locking modal.
How?
After chatting with @mtias at WCEU, we decided to try and implement this feature using the
templateLockattribute for container blocks. ThetemplateLockis already supported by API and provides a native way to inherit lock status.You can find more details on how the
templateLockand block lock work together can be found in the original Dev Note.Limitations
templateLockvalue. E.g., If you decided to restrict only movement and apply this to inner blocks, nothing would happen.Testing Instructions
Screenshots or screencast
CleanShot.2022-06-23.at.08.44.56.mp4