-
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
Try: Reusable block edit locking #39950
Conversation
Size Change: +1.32 kB (0%) Total Size: 1.22 MB
ℹ️ View Unchanged
|
The PR needs polishing, but I decided to share the current progress for feedback. Cc @paaljoachim. |
Great! Thanks George for getting this rolling! Sharing thoughts... Pre existing Reusable blocksI added an existing Reusable block. With the new lock mechanism I would expect to see existing Reusable blocks automatically locked. Plus seeing the lock icon in the toolbar. Currently adding an existing Reusable block looks like this: I would expect to see the lock in place and automatically have all current Reusable blocks content restricted. That means adding an existing Reusable block should automatically look like this: It would make it easier for sites that have a lot of Reusable blocks automatically have the lock in place. Compared to needing to manually add the lock to all the Reusable blocks. EDIT: Creating a new Reusable blockShould we add a checkbox into the Reusable block modul to restrict editing? Currently: All Reusable blocks should show the lock icon (open or closed) in the toolbar to give the needed hint for users that this can be locked or unlocked. Because Reusable block is such an important feature that one needs to remember to lock it to keep it safe from accidental changes. |
Nice, this is looking good so far! Curious love to hear what others think about the overlay — it does seem to fit nicely here but I could also see it working without. @Mamaduka, would be possible to also make some tweaks to the modal in this PR? I made a few refinements to clean things up a bit: Changes made in this version:
I think the first two changes could be made universally wherever we use nested checkboxes like this (the Preferences modal for example) since a few folks have commented about how busy the separators are. Here's the latest in the Figma file for reference. @paaljoachim That's an interesting idea above about surfacing the locking option earlier in the Reusable block set up flow — I could see that being useful! |
Hey @critterverse Channing I really like the new modal! -- I might still keep the "Restrict editing", "Disable movement" and "Prevent removal" to enforce and strengthen the action of each option. Regarding the overlay.I see it as strengthening the lock mechanism. As it shows the content that is the Reusable block (locked or unlocked). An example with a locked Reusable block. |
7925d8a
to
6057b86
Compare
@critterverse considering that those are general changes for Block Locking Modal, I created a separate PR - #39998. @paaljoachim, I would love to hear others' thoughts on Reusable Blocks being locked initially. cc @mtias, @youknowriad
A lock icon in the toolbar means that the block is locked. Displaying it only for Reusable Blocks in any other state can create confusion. |
To not break consistency instead of having an unlock icon in the Reusable block toolbar we could just automatically add the lock to all current Reusable blocks. (As it would make it easier for sites that have a huge number of Reusable blocks. I am though a bit forth and back on auto locking current Reusable blocks.) Btw |
Thanks again for the feedback, @paaljoachim. Let's wait for what others have to say about initial locks.
I would rather keep this PR simple and only focus on Edit locking for reusable blocks. |
I think it's probably a good idea to explore the default lock status separately. On the surface it may seem a good idea, but there is nuance to consider – is the locking local or global? What level of locking do we apply, just movement, just editing, everything? Should there be an Editor setting to configure the default? There are also initiatives like #39281 which may change how we feel. |
06824cc
to
b168d03
Compare
Rebased PR, so it includes the latest modal design update (#39998). I think this is ready for another round of reviews 🙇 |
} ); | ||
}, [ canMove, canRemove ] ); | ||
}, [ canEdit, canMove, canRemove, isReusable ] ); | ||
|
||
const isAllChecked = Object.values( lock ).every( Boolean ); |
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.
could be replaced with isLocked
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.
The effect uses these values, not derived one.
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.
LGTM and works as expected. Noting that this only locks a specific instance of the reusable block. I can still go to another page and change it there, or even add it the second time in my currently opened editor.
Thanks for the reviews. There's a plan to explore default lock states separately - #39950 (comment). |
I look forward to protecting the Reusable blocks globally, as it feels like expected behavior to lock a Reusable block and it will also transfer over to other instances. I had not noticed this behavior before today. As this seems kinda like a bug.
Could we fix this during the beta @adamziel ? |
To me, that's a scope extension and would probably involve some updates to the REST API. I'd say it doesn't pass the feature freeze test. If that's a blocker, then let's punt this to 6.1. If it's not, let's get it in as is. |
Nice catch, @tomasztunik. I will add this to my follow-up PR list. |
Alrighty. We are going one step forward, and have one step left to better protect Reusable blocks. |
* Introduce canEditBlock selector * Update BlockContentOverlay * Update UI * Fix indeterminate state and e2e tests * Update list view * Update selector * Provide 'isLocked' value via hook
This PR was cherry-picked for the Gutenberg plugin v13.0 release and WordPress 6.0 RC1 release. |
@bph this feature is removed from 6.0, so I will remove the "Needs Dev Note" label. |
Thanks and I remove it from your list of Dev Notes :-) |
Will this be in WP 6.1? |
Hey @GaryJones Various PRs that are merged into the Gutenberg plugin will be added to the next major version of WordPress. So yes this will be added to WP 6.1. |
What?
Closes #32461.
PR adds an option to lock editing for Reusable Blocks.
Kudos to @thisissandip for working on the initial implementation.
Why?
It is easy to modify and save reusable blocks, and then these changes are reflected across the site.
How?
The
BlockContentOverlay
component checks if the block editing is enabled. If a block cannot be edited, the overlay is permanently active.The block editor can use this implementation to enable edit locking for other blocks, like "Template Parts," that use
BlockContentOverlay.
Testing Instructions
Note: Currently, inner blocks can be removed or re-arranged via List View.
Screenshots or screencast
CleanShot.2022-03-31.at.18.44.12.mp4