Block Editor: Introduce block locking UI#39183
Conversation
|
Nice! This is really solid even in this iteration, here's a GIF showing the ellipsis locking method and the lock in the toolbar: I'd love thoughts on this by @critterverse, I know she's put a lot of thought into the feature. To me this looks great. One thought is when you select multiple blocks and one of them is locked. We might want to show a "partial lock" icon in the block toolbar. I imagine a lock icon with a minus integrated somehow, just like the checkbox behavior inside. It also seems like there's some indentation stuff we can polish inside the modal, happy to help there next week. But overall, this is very nice! 👏 |
|
Size Change: +1.36 kB (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
|
This is great. Thank you for working on it 👍🏻 |
|
Awesome, thanks for spinning up this PR so quickly @Mamaduka! It’s working great. A few small things we could update:
(Re: Pablo's note about changing the line height for text in the modal — this may be something we want to look at more universally and could be addressed separately)
Love this idea and I’m happy to help create this asset — @jasmussen, can you help us confirm whether the lock icon used in this PR is the most up-to-date version (see this comment for context)? I believe some work was done on this icon recently so just want to make sure I’m using the right one to create the new partial lock version 😁
+1 Also wondering if the block icon and lock icon could be a bit closer together (like the position of the block icon and drag handles) if we have some flexibility there — but I will defer to @jasmussen who is has done a lot of work with block toolbars on the correct spacing! |
packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
Outdated
Show resolved
Hide resolved
|
Thanks for the great feedback, folks! @critterverse I pushed updates based on your feedback. This is the only remaining item:
It looks like a separate issue, and I'll create a new PR. |
|
@mtias do we want to support locking multi-selected blocks? |
|
@Mamaduka no. We might want to allow a parent block to lock all its children in one go, though. |
This probably should be handled by |
299735a to
80cc975
Compare
97fce72 to
442b0fd
Compare
youknowriad
left a comment
There was a problem hiding this comment.
This is looking good. Aside the removal of the exposed components which seems useless to me, the rest of the comments are just small remarks.
I noticed two weird things related to locking (kind of unrelated to this PR):
- I can still move the block by moving its siblings
- I can still move the block inside a container by "grouping" it.
I feel both of these should probably be restricted? cc @senadir
|
|
||
| .components-modal__frame { | ||
| @include break-small() { | ||
| max-width: $break-mobile; |
There was a problem hiding this comment.
Is this pattern for defining a max width copied from some place? Maybe we should support defining a width as a prop to avoid recreating the same things over and over in different modals.
There was a problem hiding this comment.
It is the new recommended max-width for action modals. It might be a good idea to make it default or, as you suggested, allow setting it via prop.
cc @jasmussen
There was a problem hiding this comment.
Definitely good to absorb in the mechanics of the modal component to streamline the use and avoid arbitrary values.
There was a problem hiding this comment.
I'm just worried about this introducing a regression for Modal users that needed a different maxwidth`.
@mirka , do you think we could apply our style deprecation strategy for this?
There was a problem hiding this comment.
It is the new recommended max-width for action modals
Are there other notable kinds of modals other than "action" modals? Or can we take this statement to mean that this is the new recommended max-width for basically every modal in the codebase?
If it's the latter, we'd probably want to initiate a style deprecation to make this the new default.
There was a problem hiding this comment.
We could also treat these as a "compact" variation you opt-in into and leave the default as the almost full-width one
There was a problem hiding this comment.
Even for the template part placeholder modal, I had to tweak the width in a similar way (I copied the code from the preferences modal). I think at the component level we should come up with conventions (props, variations or something like that) that everyone should follow and address all these use-cases.
packages/block-editor/src/components/block-settings-menu-controls/index.js
Show resolved
Hide resolved
I don't think we can avoid this without locking the full content template.
Maybe we should disable Grouping like we disable transformations when removal is locked. |
That's fine to my use case. I think we have two use cases when it comes to locking movement:
I see movement locking as something useful if you want to keep the order of several blocks the same. You can use the API with a negative value
I think this falls outside the block scope? I'm not sure if we want to restrict grouping but it might be a good next step? |
Grouping is a transformation, so if we disable transformation, we should disable grouping as well IMO |
|
Thanks for the feedback and reviews, everyone. I'm going to merge this. We can address the remaining small items in follow-up PRs. However, they seem to be more general enhancements than tied to this feature. |
|
Great work @Mamaduka and @critterverse! I think the next follow up should be ensuring you can disable the ability to lock / unlock globally or per user role. |
|
Hey @Mamaduka. Thank you for working on this, but it seems this PR caused a regression, mainly, our use case is no longer working, there's no forced locking, locking is now optional. I'm not sure if Matias' comment above about disabling locking globally is on a block basis or website basis. Is there a plan to get system locking back in place or not? We currently have a workaround for deleting blocks, but not for reordering them. locking.mov |
Description
Part of #29864.
PR introduced block locking options modal as described in specs here - #29864 (comment). Users can access it via the Block Settings dropdown menu.
Note: I omitted the "Edit" locking option. It is because it's not supported by API yet, and I think its rollout should be more granular - maybe we can start with Reusable and Template Part blocks.
Questions
templateLockattribute set, should lock icon appear for child blocks?Todos
Testing Instructions
Screenshots
Block.Locking.UI.mp4
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).