Introduce Block type level lock control #32457
Conversation
senadir
requested review from
ajitbohra,
chrisvanpatten and
ellatrix
as code owners
senadir
added
[Feature] Block API
| { canRemove && ( | ||
| <MenuGroup> |
There was a problem hiding this comment.
I switched the order here between MenuGroup and isLocked (now canRemove) because when removing was disabled, there would be an empty cell there.
| return ( clientIds, rootClientId ) => { | ||
| return { | ||
| return function* ( clientIds, rootClientId ) { | ||
| const canMoveBlocks = yield controls.select( |
There was a problem hiding this comment.
I wasn't sure how would mixed blocks behave, so I follow what was done with duplication. If you have several blocks selected and one of them is locked (can't be removed or moved) then the whole action is prevented.
There was a problem hiding this comment.
This seems to be the right approach 👍
|
Size Change: +3.25 kB (0%) Total Size: 1.04 MB
ℹ️ View Unchanged
|
| // it is not possible to move the block to any other position. | ||
| if ( templateLock === 'all' ) { | ||
| // If one of the blocks is locked or the parent is locked, we cannot move any block. | ||
| if ( ! canMoveBlocks ) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
This check is already contained inside canMoveBlocks
| // undefined blocks can still be deleted. | ||
| if ( ! blockType ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
My assumption here was that an undefined block type means the block is not registered, but it's still a block, so it can be removed.
However, this ignores if the parent is locked or not. I can remove this and make all undefined blocks removable but it feels more like restrictions.
There was a problem hiding this comment.
Seeing this, all of this block (1404 to 1414) can be removed, it would always differ to the parent lock status.
|
Hi @senadir, Using this API if a block defines: The block can be inserted normally using the inserter but then the user can not remove it anymore. Is this a case we want? The locking for the move is also something that does not depend only on the block, e.g: I may move a block by moving its neighbors. To me, it seems if a block can be moved or removed is not a property of the block but of the context where it is e.g: parent block or post type template.
For blocks, with complex logic like a checkout, I imagine these blocks may need to have some type of validation. E.g: the container block verifies if all the step blocks are inside and if not shows a red alert saying block x is missing and maybe even provides a button that allows the user to press it and fix the situation (e.g: inserts the missing block). This allows the user do everything while building its checkout e.g: delete things etc and then when finished the user adds what's pending. The validation logic still allows things like removing a block that should not be removed (although the parent becomes invalid). But what if we expand inner blocks to besides template block also allow the parent block to pass some kind of function that given a block id of a child block defines if the block can be removed or not? This would allow us to have advanced inner block lockings and implement very complex rules. Basically, the main question is should be locking be the property of the context of a block (e.g: its parent) as it is now, or should it also be a property of a block? If it is just a property of the context of a block I guess we can expand InnerBlocks to allow more complex lockings than the templateLock we have now. |
|
I think I can chime in a little about the benefits of this being block-level @jorgefilipecosta
Both :)
This is only partly true. You can indeed move unlocked neighbours, but you cannot move the original locked blocks. So for instance, take a scenario where we have the following blocks: If the steps are locked, you can move the
We're hoping to prevent this level of control so the experience is curated and cannot be "broken" by a merchant, whilst enabling themselves and extension developers the ability to add components where needed. Validation is still something we may have to do in the interim, but this is not ideal in our use case. It would be a very poor UI to show move and remove controls on Blocks if using those controls immediately invalidate the parent.
I'm not sure if this is a problem if the API is clear this is a side-effect. We're able to prevent the insertion of these locked blocks in other ways, and just define them in the default template. |
|
Hey there Jorge! I think Mike provided a good answer to all of your questions, but let me expand a bit on a couple of them.
This is something I did consider, and so did @jameskoster and @mtias, here's Matias respond around it:
Alongside the use case that Mike provided.
This is also something I debated with myself and didn't get to a clear solution yet. According to the original issue, and our use case, those type of blocks are going to be pre-inserted into templates or innerBlocks structures, patterns and so on. This might not always be the case, and we to consider other possible scenarios.
It is indeed both as Mike mentioned, parent locks is a global attribute to all innerBlocks and per block is an exception or an enhancement to that lock.
That was my original plan actually but I did run into limitations with it, after a discussion with Matias we decided that having it on the block level is the best approach. |
|
Thank you @mikejolley and @senadir for detailing the use case and for all the insights provided.
Could you provide more details into the limitations that this approach had? Using block supports also has some limitations e.g: the locking affects all block instances no matter where the block is nested on, and the locking could not be dynamically changed. I wonder if we could implement the equivalent functionality using attributes e.g: a lock object or lockMove, lockDelete attributes when true the block is locked. This would allow the locking to be totally dynamic e.g: the parent block can control the locking of the children. Is more powerful as we can have the same block locked in some cases and unlocked in others. It seems to fit your use cases very well the parent block can configure the locking of the children using the template that pre-inserts the blocks.
The attribute approach also goes in the direction of locking being dependent on the block and on the context. Using an attribute locking can be changed dynamically from the block, the parent, children, and or siblings. Any thoughts on this direction? In either direction, I guess initially we should go with an "__experimental" to allow us to try it and make sure it is the right choice. |
Just wanted to share that we already use in several places block attributes to control the block inside the editor rather than the content or visual appearance as folks would assume. Some examples: gutenberg/packages/block-library/src/column/block.json Lines 16 to 18 in c405fe0 gutenberg/packages/block-library/src/group/block.json Lines 14 to 16 in c405fe0 There is an open PR #31326 that proposed adding It even looks like |
Oh I remember that one... #25183 was merged by mistake while trying to resolve a merge conflict 😬. It was immediately reverted (actually I think there was a hard-push on trunk within seconds in coordination with Riad) and the PR was re-submitted in #25778, so you can ignore it here 👍 |
What we have now are attributes to control the locking of descendants, and allowed blocks via the parent attributes. What I'm suggesting expanding that attribute mechanism and have attributes to control the ability to remove and move a specific block. |
One limitation is that locking would be applied on all children so validation would run on two dimensions: It also doesn't offer the same granular control provided by block-per-block locking.
This PR only covers the "system level" locking mentioned in the original PR, it probably makes more sense for blocks who have A follow up PR would introduce the "user level" locking which would work on attributes themselves and would fulfill the needs you mention. For locking attributes to be dynamically inserted into the block, I assume we still need some sort of support level? So the general idea I have is for lock to have three values
The only limitation I see here I that you can't definitely lock a block instance, having it on attributes would also make it switchable. If we can make an instance definitely locked (with an attribute you can't change) then we might be able to skip the system locking level. |
|
I think having locking attributes in a block and allowing the user to lock and unlock are two different things. E.g: I may want to use the attribute system so locking can be controlled by templates (like we have for allowed blocks and templateLock in columns and groups) but I may not want to allow the user to change the locking.
I guess we can have a block definitively locked by setting the locking attributes to be locked and not enabling the UI system. The user would have no way to change the attribute. We may not need a supports system at all the attributes can be explicitly added by the block as we do for templateLock: In this case, it is even simpler as everything is a boolean. Adding an attribute is almost the same effort as adding a supports flag. It would also allow each block to define the default value of the attribute, e.g: if it is locked by default. Regarding the ability for the user to configure locking it could also be another attribute. If the locking is user-configurable for a block and the fact it is user-configurable is static it would never be possible to reliable lock that block inside a CPT template (the user could just unlock it). If the fact the user can change the locking attributes is another attribute, on a CPT template we could just set that attribute to false. In summary, what I'm proposing are three attributes whose names we can decide:
Blocks can set default values for these attributes (using default:), templates CPT's can also freely change these attributes. @senadir what are your thoughts on this approach? Do you think it would work for your use case? |
|
I'm not following the development of Gutenberg closely but how common are standard predefined attributes? In my experience, all extra features (like typography and colors) are unlocked via the supports property, right? In both cases, I do agree this solution is the preferable one, but it does increase the number of attributes to N+1. N being what's being locked (moving, removing, editing) and +1 is to unlock. This also limits the user-configurable property to be all including, e.g allowing locking and unlocking the moving would also unlock removing, same for editing. This is not a problem for me because I don't want for my blocks to be user-configurable but other developers might come back later and want to have more gradual control which would make this 2N. |
|
@jorgefilipecosta fixed test. Should we go and merge? |
| const props = { | ||
| isFirst: false, | ||
| isLast: true, | ||
| canMove: true, | ||
| numberOfBlocks: 2, | ||
| firstIndex: 1, | ||
|
|
||
| onMoveDown: jest.fn(), | ||
| onMoveUp: jest.fn(), | ||
| onLongPress: jest.fn(), | ||
| onMoveDown: jest.fn(), | ||
| onMoveUp: jest.fn(), | ||
| onLongPress: jest.fn(), | ||
|
|
||
| rootClientId: '', | ||
| isStackedHorizontally: true, | ||
| }, | ||
| } ); | ||
| rootClientId: '', | ||
| isStackedHorizontally: true, | ||
| }; | ||
| const wrapper = shallow( <BlockMover { ...props } /> ); | ||
| expect( wrapper ).toBeTruthy(); | ||
| } ); | ||
|
|
||
| it( 'should match snapshot', () => { | ||
| const wrapper = shallow( <BlockMover />, { | ||
| context: { | ||
| isFirst: false, | ||
| isLast: true, | ||
| isLocked: false, | ||
| numberOfBlocks: 2, | ||
| firstIndex: 1, | ||
| const props = { | ||
| isFirst: false, | ||
| isLast: true, | ||
| canMove: true, | ||
| numberOfBlocks: 2, | ||
| firstIndex: 1, | ||
|
|
||
| onMoveDown: jest.fn(), | ||
| onMoveUp: jest.fn(), | ||
| onLongPress: jest.fn(), | ||
| onMoveDown: jest.fn(), | ||
| onMoveUp: jest.fn(), | ||
| onLongPress: jest.fn(), | ||
|
|
||
| rootClientId: '', | ||
| isStackedHorizontally: true, | ||
| }, | ||
| } ); | ||
| rootClientId: '', | ||
| isStackedHorizontally: true, | ||
| }; | ||
| const wrapper = shallow( <BlockMover { ...props } /> ); |
There was a problem hiding this comment.
Those tests were never running correctly, they're fixed now.
|
Thanks for this initial rollout! Do we need to capture pending UI tasks, like showing the lock icon in some circumstances in the toolbar and list view? |
I was going to use #29864 as a reference for the remaining tasks, I will probably add a comment there with what we need. I started working on lock icon showing in list view last week but the interactions are not obvious yet, some questions:
I'm assuming, at this state, we can probably answer the first question (for both list view and toolbar). We still don't have an API yet to tell the editor that locking is toggleable. There’s also a bug I spotted last week that I should fix as well, you can drag a block from the list view, your dragging won't persist and the block won't land in its spot, but the interaction is still there and should be disabled. Should I create the todo list anyway and let the interaction discovery be part of it? |
|
Yes, we should show lock for any of the locked states. The plan was to show the lock icon only for cases where the user can interact and toggle it off. If a user cannot change the state it won't show up. A todo would be great! We can incorporate it into #29864. |
There was a problem hiding this comment.
Thank you for bringing this task to the finish line @senadir and for all the interactions. This system seems really flexible and powerful let's see what feedback the community gives.
| return settings; | ||
| } | ||
|
|
||
| addFilter( 'blocks.registerBlockType', 'core/lock/addAttribute', addAttribute ); |
|
|
||
| **Block-level locking is an experimental feature that may be removed or change anytime.** | ||
| ```js | ||
| attributes: { |
There was a problem hiding this comment.
Cool work.
It looks like the documentation needs to be fixed. It differs from the PR's description and feels broken here:
|
I'm a bit late for the party. What's the plan with gutenberg/packages/block-library/src/column/block.json Lines 19 to 21 in 01ac2cf gutenberg/packages/block-library/src/group/block.json Lines 14 to 16 in 01ac2cf Should we leave it as is or refactor to the new API? |
|
|
|
I believe placing lock feature in attributes was a poor choice. It forces developer remember a solution that is not logical and he cannot use Or even better proposal: place it in a block editor store and not block itself, and it should be reachable by utilising select / dispatch . After all, block editor should control block locking and it makes more sense to keep it there. I would also add "allowLocking" property in supports object - which would in block definition indicate if it could be locked at all. This could be true by default. |
Using locking as a property and not as an attribute means locking would not be persisted when the editor loads. We would also lose flexibility in setting locking from templates as we set all the other properties. |
|
@Lovor01 I don't know how would locking be of any value to live in block Props, but I do agree that locking is an instance-based setting, not a block type settings, so it either lives there or in attributes, but not in support because support is a block type-based option (it applies to all instance of said block). |
|
Just wanted to share my thoughts on dynamics between the global Logically I feel like the I don't know if this is the intended behavior planned for |
|
@amarinediary do you mean in correlation with block level locking? block level locking (which handles the actual block, not its children) supersedes the templateLock value. |
|
@senadir Yes exactly. As I understood block level locking will only affect the block itself not the children. But, Does level block locking will finally overwrite template locking in |
Parts of #29864
Description
This PR introduces parts of #29864 mainly, it adds:
The PR also adds checks at the datastore level for both moving and removing blocks.
API definition
When defining a block, the value is set at the attributes level.
This would lock the block ability to be moved or removed.
How has this been tested?
insert,all, orfalseto see the all the possible combinations.This table shows the intersect of conditions between
templateLockandlockFor removing:
"all""insert"falsetruefalseundefinedFor moving:
"all""insert"falsetruefalseundefinedScreenshots
In this example. the parent block is locked to
all, the first block "Contact information" has locking disabled for move and remove, which means it can be moved or removed.https://user-images.githubusercontent.com/6165348/120816789-fc87af80-c548-11eb-821e-227083a9badf.mov
Use case
There are several valid uses cases outlined in #29864
Another use case that we're building for is having a Checkout Block with different blocks that act as fundamental steps, we don't want people to delete or move those steps since they're fundamental and their order is also important, but we want to allow people to select them, access settings, and insert blocks in between them.