Show warning on critical block removal #51145
Conversation
tellthemachines
requested review from
ellatrix,
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
Size Change: +7.67 kB (+1%) Total Size: 1.42 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 6a0a806. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5348150109
|
| @@ -74,7 +77,21 @@ export default function BlockActions( { | |||
| return duplicateBlocks( clientIds, updateSelection ); | |||
| }, | |||
| onRemove() { | |||
| return removeBlocks( clientIds, updateSelection ); | |||
| const shouldDisplayRemovalPrompt = clientIds | |||
There was a problem hiding this comment.
Is this a possible candidate for memoization? I'm not sure, I guess clientIds would be constantly changing. It might be more optimal assuming a user is trying to remove the same block from the same block tree 😄
| @@ -105,7 +108,22 @@ function ListViewBlockSelectButton( | |||
| // fallback to focus the parent block. | |||
| firstBlockRootClientId; | |||
|
|
|||
| removeBlocks( blocksToDelete, false ); | |||
| const shouldDisplayRemovalPrompt = blocksToDelete | |||
There was a problem hiding this comment.
Similar thought to the shouldDisplayRemovalPrompt above.
| const message = | ||
| blockName === 'core/post-content' | ||
| ? __( | ||
| 'This block displays the content of a post or page. Removing it it is not advised.' |
There was a problem hiding this comment.
Should we indicate why it's not advised as mentioned in the issue?
I'm leaning towards "yes", however I suppose that opens the can of worms that we'd have to customize the message for every warning depending on the block. 🤔
| @@ -66,7 +69,24 @@ export default function useInput() { | |||
| node.contentEditable = false; | |||
| event.preventDefault(); | |||
| if ( __unstableIsFullySelected() ) { | |||
| removeBlocks( getSelectedBlockClientIds() ); | |||
| const shouldDisplayRemovalPrompt = | |||
There was a problem hiding this comment.
Maybe this should be something like blockNamesWithWarnings or something way better. should* makes me thing the return values is a bool
| } from '@wordpress/components'; | ||
| import { __, sprintf } from '@wordpress/i18n'; | ||
|
|
||
| export function showBlockRemovalWarning( blockName ) { |
There was a problem hiding this comment.
Hmm, I'm wondering about the mixed type return value. The function name, at least in my mind, implies a bool, but I see blockName is handy to have to display the removal prompt content.
Could we rather pass down a list of IDs and return those that required removal. That means we could memoize in one spot too. E.g.,
getBlockNamesWithRemovalWarnings( clientIds ) // => [ 'core/query' ]
/**
*
* @param {Array} clientIds
* @returns {Array}
*/
const getBlockNamesWithRemovalWarnings = ( clientIds ) =>
clientIds
.map( getBlockName )
.filter( ( blockName ) => {
return blockName === 'core/query' || blockName === 'core/post-content';
} );Would that work?
|
Thanks for the feedback @ramonjd ! Those are all great points, but I'd love to have more thoughts on the approach right now. Details can be ironed out once we're sure this is the way to go 😅 |
There was a problem hiding this comment.
Nice work, it's testing pretty well!
One thing I noticed is that this applies in the post editor, too, so if you're editing a post or page that contains a Latest Posts section and go to remove the Query block, the warning will also display. Is that intended? Would it be worth further limiting the warning so that it only displays when editing a template or template part?
This is a first approach: it works, but the code is messy.
In terms of the logic, I actually quite like it, and that it's explicit about when it's being executed, rather than on every call of removeBlocks. The main thing I was wondering from looking over the code is whether we can abstract the logic into a hook? That way, if it's only an extra couple of lines in each place it's used, I'm wondering if that might make it feel less messy when it has to be manually introduced in each place where we need it? 🤔
should this initial version be extensible?
Personally, I quite like the hard-coded idea to begin with, since the idea of the feature is quite tied to the idea of what is required in a template in order for it to function. But I could imagine potentially having a property in block.json or something like that in the future, if folks wanted to have a flag to say "warn on removal", like you mention.
| title={ sprintf( | ||
| /* translators: %s: the name of a menu to delete */ | ||
| __( 'Remove %s?' ), | ||
| blockName |
There was a problem hiding this comment.
Can we grab the human friendly (display) name here? Maybe via useBlockDisplayInformation, since currently it shows up as core/query:
Sounds good — apologies if I went too detailed, too! |
|
Just another thought for extensibility — I wonder if it'd be helpful storing the list of critical blocks to warn on removal in state somewhere? For example when core blocks are registered, |
Agree with @andrewserong, I think it'd be great to keep a smaller footprint and even make the store methods private for now until things are stabilized.
I agree with your instinct. Until there are more blocks on the warning list, I'm assuming it might be more optimal the way you've done it (?)
You bet! |
|
This is looking great, but I do want to call out the need for extensibility in the long run. I can imagine people wanting to add additional blocks to this "critical" list. I'm thinking about those that are building custom blocks for clients or perhaps e-commerce blocks. There also needs to be a way to turn these warnings off. I think this is probably more important than extensibility right now. While helpful to the average user, I can see these warnings becoming a bit annoying for power users or theme builders who are in the process of building out a block theme. |
tellthemachines
added
the
[Feature] Site Editor
Good point 😄 I think most admins would turn it off immediately, or soon after they become familiar with blocks. I suppose the warning should furthermore only arrive on the scene for already-published blocks, or blocks with changes that have been saved once? Otherwise we see something like this: 
I'm also wondering whether a warning for the query block (for example) should be shown at all in the post editor 🤷 I guess making it extensible would allow site owners to build their own rules. Especially given that the premise behind the feature, that there are some blocks that one shouldn't remove, in my mind at least, is situational and could be unique to every site. Less intrusive than a popup could be a notification that appears after the deletion of target blocks and offers and "Undo" step 🤷 
|
|
Thanks for all the thoughts folks ❤️
I didn't really consider it! It doesn't seem as necessary on posts or pages, because it's doubtful Query will be the main content there. I'll look into limiting the prompts to templates for now. That's also something we'll need to consider whether to make customisable or not.
Something like
No need to apologise! I just needed to work out the high-level shape of things before starting to address the finer points 😄
Yeah, I think the extensibility bit will need some more thought. We'll likely need extenders to provide a custom message for the removal warning too.
👍 It might not even be necessary to add the removal prompt logic anywhere else; that's probably something that can be worked out once there's a public API and we start seeing what kind of real world usage it gets.
Good point! Let's get some testing feedback on the initial version and see how best to approach configurability in the UI. I'm thinking it could be something like a "Don't show again" checkbox in the modal itself. |
The problem is we don't want to accidentally remove empty Post Content blocks - that's probably a more common occurrence than removing Query blocks. I think this is one of those things that we'll have to road test and smooth out iteratively. |
|
Thanks for the PR @tellthemachines!
I definitely agree with @andrewserong about this. This seems like it should be a part of Blocks API and not hardcode some block names and messages in block editor package. This API in my mind would need to be aware of context(site editor, post editor, template, etc..) and provide different messages conditionally. Being a Block API means it will be also filterable with the existing filters for block settings. |
Sure, the consensus is that there should be an API, but for the initial experiment it's better to just hardcode the block names so we can quickly get something out there to be tested, and get some feedback to help inform what kind of API is needed.
I've been thinking about how best to provide this context. If there is to be an API that allows extenders to define third party blocks as "critical" and provide a message for the prompt, it would make sense that extenders can also configure which editors the prompt displays in. The other thing to consider is if this API should also work with non-WP block editors, in which case we shouldn't hardcode editor context either. |
|
I've now put all the logic that determines whether to show the prompt in a hook which returns a function that can be called instead of Regarding the problem of only displaying the prompts in the site editor, I tried moving the modal into the site editor itself, around here but it's not working correctly. I'm not sure if there's a better place to put it in the site editor, or if there's another reason it doesn't work. Will continue looking into it tomorrow, but any ideas welcome in the meantime! |
There was a problem hiding this comment.
Hi! I left a mishmash of inline comments that reflect my thought process as I reviewed. That means that the comments at the very end (chronologically) are probably the most important ones. I hope that the most important questions are apparent, but if it's confusing please let me know and I can try to rephrase or sum up my thoughts. :)
| const { isSelected } = useSelect( | ||
| ( select ) => { | ||
| return { | ||
| isSelected: | ||
| select( blockEditorStore ).isBlockSelected( clientId ), | ||
| }; | ||
| }, |
There was a problem hiding this comment.
Oh, I should revert that! I had a couple more selectors in there but removed them with the latest commit.
| removalFunction: () => { | ||
| removeBlocks( clientIds, selectPrevious ); | ||
| }, |
There was a problem hiding this comment.
Is the removal function ever anything other than removeBlocks( clientIds, selectPrevious )? Looking at the branch, it doesn't seem so. Seems like one piece of state that could be removed from isRemovalPromptDisplayed and one inch of API surface reduced.
There was a problem hiding this comment.
It's always removeBlocks, the only difference is whether selectPrevious is true or false. I guess we could just pass selectPrevious instead of the whole function.
| }; | ||
| } | ||
|
|
||
| export function BlockRemovalWarningModal( { |
There was a problem hiding this comment.
Should probably be under ../components
| closeModal, | ||
| removalFunction, | ||
| } ) { | ||
| const message = |
There was a problem hiding this comment.
The messages should sit next to the corresponding block types. Something like:
const blockTypePromptMessages = new Map([
[ 'core/query', __( 'This block ... ' ) ],
[ 'core/post-content', __( 'This block ... ' ) ],
]);to be shared by both hook and component.
(I know others have suggested making this pluggable, but I personally wouldn't rush it.)
There was a problem hiding this comment.
Agreed, no hurry to create a public API for this.
| <Button variant="tertiary" onClick={ () => closeModal() }> | ||
| { __( 'Cancel' ) } | ||
| </Button> | ||
| <Button variant="primary" onClick={ () => onConfirmRemoval() }> | ||
| { __( 'Confirm' ) } | ||
| </Button> |
There was a problem hiding this comment.
| <Button variant="tertiary" onClick={ () => closeModal() }> | |
| { __( 'Cancel' ) } | |
| </Button> | |
| <Button variant="primary" onClick={ () => onConfirmRemoval() }> | |
| { __( 'Confirm' ) } | |
| </Button> | |
| <Button variant="tertiary" onClick={ closeModal }> | |
| { __( 'Cancel' ) } | |
| </Button> | |
| <Button variant="primary" onClick={ onConfirmRemoval }> | |
| { __( 'Confirm' ) } | |
| </Button> |
| if ( innerBlocks.length ) { | ||
| return findFirstCriticalBlock( innerBlocks ); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
Minor point, but following find convention, this could/should be undefined.
| */ | ||
| import { store as blockEditorStore } from '../store'; | ||
|
|
||
| function isBlockCritical( blockName ) { |
There was a problem hiding this comment.
See comment about blockTypePromptMessages
| displayPrompt: _displayPrompt, | ||
| removalFunction: _removalFunction, | ||
| blockToRemove: blockName, |
There was a problem hiding this comment.
See my other comment about the need for a removal function. If we could distill the data needs here, I think we could reduce this to a nicer:
| displayPrompt: _displayPrompt, | |
| removalFunction: _removalFunction, | |
| blockToRemove: blockName, | |
| shouldDisplayBlockRemovalPrompt: !! blockToBeRemoved(), |
Where select( 'core/block-editor' ).blockToBeRemoved is the successor to isRemovalPromptDisplayed and only returns the block name if there is one, or null/undefined.
There was a problem hiding this comment.
Looking at this again, I wonder if this state should even be read/handled by BlockList at all. Shouldn't BlockRemovalWarningModal just handle everything internally? We have many such components:
<>
<elementContext.Provider value={ element }>
<IntersectionObserver.Provider value={ intersectionObserver }>
<div { ...innerBlocksProps } />
</IntersectionObserver.Provider>
</elementContext.Provider>
<BlockRemovalWarningModal />
</>where BlockRemovalWarningModal might become BlockRemovalPromptListener or anything that better conveys its conditional nature.
There was a problem hiding this comment.
Yeah good point, it'll be cleaner if the modal itself gets what it needs from state. As it stands, the modal needs to know both the name of the block to be removed, and the function to call if the user confirms the action.
There was a problem hiding this comment.
Shouldn't BlockRemovalWarningModal just handle everything internally?
I've been playing with this a bit but can't quite get it to work, I think because if there's no state change in the block list when a block deletion is attempted, there's no reason to re-render the component so the modal never gets triggered when it's needed. Plus it renders a bunch of times when the block list first loads which is kind of annoying 😅
There was a problem hiding this comment.
I gave this a go in f0247fe. This works, right?
There was a problem hiding this comment.
Yes! I was pretty much trying to do that, so not sure what went wrong 😕 probably had a typo somewhere. Thanks!
| export function displayRemovalPrompt( displayPrompt, options = {} ) { | ||
| const { removalFunction, blockName } = options; | ||
| return { | ||
| type: 'PROMPT_REMOVAL', | ||
| displayPrompt, | ||
| removalFunction, | ||
| blockName, | ||
| }; | ||
| } |
There was a problem hiding this comment.
I wonder if we can / want to fold this logic into the removeBlocks action, since that's the central code path for removal and it already is the site for checks and conditional dispatching:
- checks for
canRemoveBlocksbefore proceeding - dispatches
selectPreviousBlockalongside the removal action if requested - ensures correct focus by dispatching
ensureDefaultBlockat the very end
I'm not sure of this idea, so I'd like to be challenged, but what about something like:
export const removeBlocks =
- ( clientIds, selectPrevious = true ) =>
+ ( clientIds, selectPrevious = true, forceRemove = false ) =>
( { select, dispatch } ) => {
...
if ( ! canRemoveBlocks ) {
return;
}
+ if ( ! forceRemove && needsConsentForRemoval( blockIds ) ) {
+ dispatch( { type: 'PROMPT_FOR_BLOCK_REMOVAL', blockIds } );
+ return;
+ }
...
dispatch( { type: 'REMOVE_BLOCKS', clientIds } );
+ dispatch( { type: 'PROMPT_FOR_BLOCK_REMOVAL', null } );
...
};then the modal would dispatch a new remove action with forceRemove: true
There was a problem hiding this comment.
Yeah, I thought about that (see my little monologue in the PR description 😄) but hesitate to do it because removeBlocks gets called in a lot of places (itself or via removeBlock) where the prompt isn't relevant, and that would mean going through the logic to check if there's a critical block in the list of blocks to remove every time.
It does feel conceptually tidier to have everything together in removeBlocks though.
I'm also happy to hear more thoughts about this!
| export function isRemovalPromptDisplayed( state = false, action ) { | ||
| switch ( action.type ) { | ||
| case 'PROMPT_REMOVAL': | ||
| return { | ||
| displayPrompt: action.displayPrompt, | ||
| removalFunction: action.removalFunction, | ||
| blockName: action.blockName, | ||
| }; | ||
| } | ||
|
|
||
| return state; | ||
| } |
There was a problem hiding this comment.
I believe this could come down to something like
case 'PROMPT_FOR_BLOCK_REMOVAL':
return action.blockIds;with the corresponding name change.
| return ( | ||
| <Modal | ||
| title={ sprintf( | ||
| /* translators: %s: the name of a menu to delete */ |
There was a problem hiding this comment.
| /* translators: %s: the name of a menu to delete */ | |
| /* translators: %s: the title of a block to delete, e.g. "Post Content" */ |
|
Going back to the problem of getting the prompt to only display in the site editor, I just pushed an update that moves the modal into the site editor, and dispatches and action from inside the modal, flagging that the modal is available. Then the hook that checks if the prompt needs to be displayed also checks if the modal is available, and if it isn't it just goes ahead and removes the blocks. This is so that the prompt doesn't display when a Query block is removed from the post editor, and it was the best way I could find of doing it without requiring the block editor package to be aware of its context (post or site editor). Thoughts for improvement or other ideas welcome! Still to do:
|
Well that was easy. :)
| // In certain editing contexts, we'd like to prevent accidental removal of | ||
| // important blocks. For example, in the site editor, the Query Loop block is | ||
| // deemed important. In such cases, we'll ask the user for confirmation that | ||
| // they intended to remove such block(s). | ||
| // | ||
| // @see https://github.com/WordPress/gutenberg/pull/51145 | ||
| export const blockTypePromptMessages = { | ||
| 'core/query': __( 'Query Loop displays a list of posts or pages.' ), | ||
| 'core/post-content': __( | ||
| 'Post Content displays the content of a post or page.' | ||
| ), | ||
| }; |
There was a problem hiding this comment.
Right now this entangles block-editor with edit-site, since these are rules that only apply to the site editor. I don't want to hold up this PR any longer, but a future improvement could be to let the consumer (the component rendering BlockRemovalWarningModal) provide their own rules. Maybe:
<BlockRemovalWarningModal rules={ myPromptMessages } />As you can tell, I'm undecided on the terminology: are these messages? Rules? Warnings?
|
Tests are failing across Gutenberg, potentially solved by #51790. In the meantime, I'll just merge this one. Thanks, everyone! (Edit: E2E tests were fine when run locally.) |
| // FIXME: Without this existence check, the unit tests for | ||
| // `__experimentalDeleteReusableBlock` in | ||
| // `packages/reusable-blocks/src/store/test/actions.js` fail due to | ||
| // the fact that the `registry` object passed to the thunk actions | ||
| // doesn't include this private action. This needs to be | ||
| // investigated to understand whether it's a real smell or if it's | ||
| // because not all store code has been updated to accommodate | ||
| // private selectors. | ||
| select.isRemovalPromptSupported && | ||
| select.isRemovalPromptSupported() |
There was a problem hiding this comment.
@jsnajdr: I quickly chatted with Adam about this. Is it possible that we are missing something in our Jest setup to accommodate private selectors?
There was a problem hiding this comment.
It's not because of Jest, but because of the cumbersome way how we register the block-editor store, with the deprecated registerStore function, forced to use this way by the persist plugin.
export const store = createReduxStore( STORE_NAME, {
...storeConfig,
persist: [ 'preferences' ],
} );
// We will be able to use the `register` function once we switch
// the "preferences" persistence to use the new preferences package.
const registeredStore = registerStore( STORE_NAME, {
...storeConfig,
persist: [ 'preferences' ],
} );
unlock( registeredStore ).registerPrivateActions( privateActions );
unlock( registeredStore ).registerPrivateSelectors( privateSelectors );This code:
- creates a
storestore descriptor for theblock-editorstore. - instantiates and registers the
block-editorstore, but using a different descriptor, the one created internally insideregisterStore. - registers private actions and selectors to that one instance (!) that was just created.
The unit test that is failing is creating a new registry, and registers (instantiates) a block-editor store in this registry, using the store descriptor.
But nobody ever added the private actions and selectors to this descriptor!
The short-term solution is to register the private actions/selectors also to store. The long-term solution is to finish and merge #39632 (by @talldan), stop using the persist plugin, and stop using registerStore for block-editor.
As a workaround, until #39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: #51145 (comment) Props jsnajdr
…52088) As a workaround, until #39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: #51145 (comment) Props jsnajdr
Following up on #51145, this untangles `edit-site` from `block-editor` by removing the hard-coded set of rules `blockTypePromptMessages` from the generic `BlockRemovalWarningModal` component. Rules are now to be passed to that component by whichever block editor is using it. Names and comments have been updated accordingly and improved.
* Block removal prompt: let consumers pass their own rules Following up on #51145, this untangles `edit-site` from `block-editor` by removing the hard-coded set of rules `blockTypePromptMessages` from the generic `BlockRemovalWarningModal` component. Rules are now to be passed to that component by whichever block editor is using it. Names and comments have been updated accordingly and improved. * Site editor: Add e2e test for block removal prompt
* Show warning on critical block removal * Extract prompt display logic into a hook * Revert formatting change. * Prompt for removal of all the blocks * Move prompt state handling out of BlockList and into self * findCriticalBlocks: don't dismiss children of matching node * Refactor by embracing `flatMap` semantics * When `isBlockCritical( blockName )`, don't immediately return: there could be other matching block types within that node's inner blocks. * findCriticalBlocks -> getBlocksToPromptFor The intention here was to drop the "find" prefix, which can mislead the render into believing the function is meant to return a single match or none. * Drop isBlockCritical() * Redesign removal modal This is an attempt to reconcile a desire for clarity in the UI as to what is about to be deleted (and why that matters) and the i18n constraints that apply: * Remove `__( 'Remove %s?' )` string, which -- although manageable -- places an added burden on translators of more inflected languages. * Trim the strings in `blockTypePromptMessages`. This allows us to stack all matching blocks' custom messages and finish off with a single "Removing it is not advised." paragraph. * Move prompt into site editor. * Reset removalPromptExists upon listener unmount * Let action removeBlocks handle prompts and confirmations * Add private action `privateRemoveBlocks` to hide extended interface * Fix unit tests * Try: Dispatch setRemovalPromptStatus in edit-site init On one hand, `initializeEditor` is where we would typically set this sort of setting, which is why this is worth trying out. On the other hand, interfering with the `REMOVE_BLOCKS` action is a big deal, so we should be absolutely sure that the prompt status in the reducer is always accurate. The `useEffect` approach has the advantage of keeping everything in one place: by rendering a single component like BlockRemovalWarningModal, we guarantee that both the setting is correct and the editor will render removal prompts. * Revert "Try: Dispatch setRemovalPromptStatus in edit-site init" This reverts commit a5cce0a. I'll repeat my reasoning from the parent commit: "[...] interfering with the `REMOVE_BLOCKS` action is a big deal, so we should be absolutely sure that the prompt status in the reducer is always accurate. The `useEffect` approach has the advantage of keeping everything in one place: by rendering a single component like BlockRemovalWarningModal, we guarantee that both the setting is correct and the editor will render removal prompts." * Make all actions & selectors private. Rename things. * Addresses pull request feedback. * See `FIXME` notes. * Make BlockRemovalWarningModal private * Cleanup: Remove BlockList changes from branch * Tweak removal message for Query. Tweak comments. * Split action into displayRemovalPrompt & clearRemovalPrompt The action types have also been renamed: DISPLAY_REMOVAL_PROMPT, CLEAR_REMOVAL_PROMPT. Simplifies the function signatures and hopefully makes everything a little bit clearer. * Rename setRemovalPromptStatus to toggleRemovalPromptSupport Also: rename the action type, then sprinkle with comments. * Rename isRemovalPromptDisplayed to getRemovalPromptData New reducer: removalPromptData New selector: getRemovalPromptData * Add missing @return to displayRemovalPrompt * Tweak modal copy per feedback * Turns out private selectors are attached to the thunk proxy! Well that was easy. :) * Don't export the new reducers * Fix tests --------- Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com>
…ordPress#52088) As a workaround, until WordPress#39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: WordPress#51145 (comment) Props jsnajdr
…52088) As a workaround, until #39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: #51145 (comment) Props jsnajdr
* Block removal prompt: let consumers pass their own rules Following up on #51145, this untangles `edit-site` from `block-editor` by removing the hard-coded set of rules `blockTypePromptMessages` from the generic `BlockRemovalWarningModal` component. Rules are now to be passed to that component by whichever block editor is using it. Names and comments have been updated accordingly and improved. * Site editor: Add e2e test for block removal prompt
* Try restoring the site editor animation (#51956) * Try restoring the site editor animation * fix header animation * Remove accidental addition of layout prop * tidy up formatting * fix animate presence issue * Fix animation between sidebar view and distraction free edit view * Leave sidebar present and maintain canvas to sidebar animation The sidebar is necessary for routing on mobile so we have to maintain its presence in the DOM. Just hiding it isn't enough though, as it is still able to be reached with keyboard tabs and screen readers. Using the relatively new `inert` property disables the element from user interaction, so we add that when we don't want the sidebar to be shown. * Fix mobile view for pattern library On Mobile, the canvas mode wasn't being set to edit when using the pattern library. This updates it to use the showSidbar value instead, keeping it in sync with the inert setting. --------- Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> * Change password input to type text so contents are visible. (#52622) * Iframe: Silence style compat warnings when in a BlockPreview (#52627) * Do not autofocus page title field in the Create new page modal dialog. (#52603) * Use lowercase p in "Manage Patterns" (#52617) * Remove theme patterns title (#52570) * Block editor store: also attach private APIs to old store descriptor (#52088) As a workaround, until #39632 is merged, make sure that private actions and selectors can be unlocked from the original store descriptor (the one created by `createReduxStore`) and not just the one registered in the default registry (created by `registerStore`). Without this workaround, specific setups will unexpectedly fail, such as the action tests in the `reusable-blocks` package, due to the way that those tests create their own registries in which they register stores like `block-editor`. Context: #51145 (comment) Props jsnajdr * Block removal prompt: let consumers pass their own rules (#51841) * Block removal prompt: let consumers pass their own rules Following up on #51145, this untangles `edit-site` from `block-editor` by removing the hard-coded set of rules `blockTypePromptMessages` from the generic `BlockRemovalWarningModal` component. Rules are now to be passed to that component by whichever block editor is using it. Names and comments have been updated accordingly and improved. * Site editor: Add e2e test for block removal prompt * Fix Shift+Tab to Block Toolbar (#52613) * Fix Shift+Tab to Block Toolbar * Add changelog entry * Show warning on removal of Post Template block in the site editor. (#52666) * Avoid copying global style presets via the styles compatibility hook (#52640) * i18n: Make the tab labels of `ColorGradientSettingsDropdown` component translatable (#52669) * Rich Text/Footnotes: fix getRichTextValues for useInnerBlocksProps.save (#52682) * Rich Text/Footnotes: fix getRichTextValues for useInnerBlocksProps.save * Address feedback * Patterns: Remove `reusable` text from menu once rename hint has been dismissed (#52664) * Show uncategorized patterns on the Editor > Patterns page (#52633) * Patterns: fix bug with Create Patterns menu not showing in site editor page editing (#52671) * Pass the root client id into the reusable blocks menu * Check that clientIds array is defined * Make check for array item more specific * Search block: Enqueue view script through block.json (#52552) * Search block: Enqueue view script through block.json * Remove extra space * Use `_get_block_template_file` function and set $area variable. (#52708) * Use `_get_block_template_file` function and set $area variable. * Update packages/block-library/src/template-part/index.php Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com> --------- Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com> * Site Editor: Don't allow creating template part on the Patterns page for non-block themes (#52656) * Don't allow template part to be created on the Patterns page for non-block themes * Remove unnecessary theme directory name in E2E test * Change Delete page menu item to Move to trash. (#52641) * Use relative path internally to include packages in dependencies (#52712) * Spacing Sizes: Fix zero size (#52711) * DimensionsPanel: Fix unexpected value decoding/encoding (#52661) --------- Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Saxon Fletcher <saxonafletcher@gmail.com> Co-authored-by: Jerry Jones <jones.jeremydavid@gmail.com> Co-authored-by: Robert Anderson <robert@noisysocks.com> Co-authored-by: Andrea Fercia <a.fercia@gmail.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: James Koster <james@jameskoster.co.uk> Co-authored-by: Miguel Fonseca <150562+mcsf@users.noreply.github.com> Co-authored-by: Haz <hazdiego@gmail.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: Glen Davies <glendaviesnz@users.noreply.github.com> Co-authored-by: Carolina Nymark <myazalea@hotmail.com> Co-authored-by: Petter Walbø Johnsgård <petter@dekode.no> Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com> Co-authored-by: Felix Arntz <felixarntz@users.noreply.github.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com>
What?
Fixes #40618.
(edited by @mcsf:)
Show a modal asking the user for confirmation when attempting to remove Post Content and Query blocks, but only in the site editor.
I have questions regarding implementation, mainly: should this initial version be extensible? If so, perhaps it would be better to register block criticality, as well as the message to display, in the block metadata. That would mean that third parties would be able to start using it immediately.For now, keep everything private using Gutenberg's new lock/unlock APIs. Everything is implemented inside the@wordpress/block-editorpackage. The@wordpress/edit-sitejust needs to unlockBlockRemovalWarningModaland render it in itsEditorcomponent.Another thought I had was should the check for whether to show the prompt be part of theYes, this works inside theremoveBlocksaction instead?removeBlockscall. We've introduced some dedicated logic, introduced a private action namedprivateRemoveBlocks, and this new logic now comes with an extra argument,forceRemove, that is enabled when the user confirms deletion via the modal.Testing Instructions
A) In the site editor, go to a template that has either Post Content or Query (both considered critical blocks) and try the following:
7. For bonus points: check if I missed any block removal methods 😄B) Make sure that there is no change in behaviour in other editors. For instance:
<!-- wp:query /-->Testing Instructions for Keyboard
Screenshots or screencast