Blocks: refactor deletion warnings dialog #58952
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
glendaviesnz
added
[Feature] Blocks
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
@annezazu , @andrewserong this PR introduced |
|
Size Change: -27 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
talldan
reviewed
Feb 13, 2024
Comment on lines
17
to
45
| 'core/query': { | ||
| message: __( | ||
| 'The Query Loop block displays a list of posts or pages, so removing it will prevent that content from displaying.' | ||
| ), | ||
| }, | ||
| 'core/post-content': { | ||
| message: __( | ||
| 'Removing the Post Content block will stop your post or page content from displaying on this template.' | ||
| ), | ||
| }, | ||
| 'core/post-template': { | ||
| message: __( | ||
| 'The Post Template block displays each post or page in a Query Loop, so removing it will stop post content displaying in your query loop.' | ||
| ), | ||
| }, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
will have another think about various approaches/naming, etc. tomorrow.
|
Have moved this back to draft as we need to take some time to think about the best API for the rules set, etc. |
youknowriad
reviewed
Feb 13, 2024
| message: __( | ||
| 'The Query Loop block displays a list of posts or pages, so removing it will prevent that content from displaying.' | ||
| ), | ||
| }, |
There was a problem hiding this comment.
Are we showing this message for the removal of all query blocks?
There was a problem hiding this comment.
In trunk it is only shown on every deletion in the site editor for query and post content - which makes sense in the context of a template, but if we are adding these rules via the editor package I think we need to somehow limit their application to posts/pages - it doesn't seem like a warning is needed in the context of post and pages for these blocks and in fact would be confusing/annoying?
There was a problem hiding this comment.
have added a postTypes option to the rules to allow setting which post types to show the rules for. Current post type is passed in via the editor provider so the block editor does not need to be know anything about the different post types.
youknowriad
reviewed
Feb 13, 2024
| message: __( | ||
| 'Removing the Post Content block will stop your post or page content from displaying on this template.' | ||
| ), | ||
| }, |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
youknowriad
reviewed
Feb 13, 2024
| blockNamesForPrompt.add( blockName ); | ||
| messageType = 'patternOverrides'; | ||
| Object.values( rules ).forEach( ( rule ) => { | ||
| if ( rule.callBack ) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
true, thanks, will fix that
|
Thanks for the follow-up here :) |
|
we will need to account for this fix in this refactor. |
glendaviesnz
commented
Feb 16, 2024
Comment on lines
18
to
30
| const blockRemovalRules = { | ||
| 'core-query': { | ||
| message: __( | ||
| 'The Query Loop block displays a list of posts or pages, so removing it will prevent that content from displaying.' | ||
| ), | ||
| postTypes: [ 'wp_template' ], | ||
| callback: ( blockName, ruleKeysForPrompt ) => { | ||
| if ( blockName === 'core/query' ) { | ||
| ruleKeysForPrompt.add( 'core-query' ); | ||
| } | ||
| }, | ||
| }, |
There was a problem hiding this comment.
@talldan I have revisited the rules to try and make them more generic, and also to add in the ability to limit the messages by post type.
So now every rule needs:
key - this is the property name and is used to map to the messages in the confirmation prompt
message - message to display
postTypes - array of post types on which to display the message
callback - method to check if warning should show - callback gets blockName, ruleKeysForPrompt and blockAttributes passed in
Doing it this way makes for a bit of repetition for simple block checks, but the advantage of just havng a callback for each is devs are then free to calculate this however they want either for specific blocks or more generic things like the pattern overrides.
This is just a first stab at this bigger refactor of this API so happy to take thoughts on alternatives.
There was a problem hiding this comment.
we can probably make the individual block callback more generic and share it between the rules somehow if we decide to go this way.
youknowriad
reviewed
Feb 19, 2024
| @@ -17,10 +17,10 @@ import { store as blockEditorStore } from '../../store'; | |||
| import { unlock } from '../../lock-unlock'; | |||
|
|
|||
| export function BlockRemovalWarningModal( { rules } ) { | |||
There was a problem hiding this comment.
Do we still want to keep this component in "block-editor"?
There was a problem hiding this comment.
Ok with moving it, but then we still have the private actions, selectors and reducers for block removal warnings in the block editor package that are all the same family of APIs. I feel like it makes sense to keep those things together.
We could consider gradually moving it all across, but it'd mean changing how this feature integrates with the removeBlocks action pretty significantly. Will have a think about whether that's possible.
There was a problem hiding this comment.
Ok, let's keep it then :)
youknowriad
reviewed
Feb 19, 2024
|
Had a chat with @glendaviesnz and have agreed to take this PR over from him as he doesn't have the bandwidth to work on this right now. |
talldan
force-pushed
the
refactor/block-removal-warning
branch
from
0104b8e to
720e370
Compare
|
I've rebased and refactored some of it that I felt could be improved. I still need to do some testing to make sure I haven't broken anything 😅 . Not sure if there are e2e tests for this feature (edit: there are, but it looks like the messages haven't been updated). The rules are now an array (to keep the displayed messages in a reliable order) of objects in this shape: {
// List of post types the message is displayed for.
postTypes: [ 'some', 'post', 'types' ],
// Callback receives array of the 'removed' blocks. If it returns a message it's displayed in the prompt.
callback( removedBlocks ) {
if ( removedBlocks.some( ( { name } ) => name === 'core/some-block' ) ) {
return __( 'Message that displays in prompt' );
}
}
},So the callback, instead of just checking eligibility for the prompt, now also returns the message itself. I think this gives a bit more flexibility if we want to do some more dynamic messages in the future, plus simplifies the The postType filtering now happens in the component instead of the I'm unsure about the way the messages themselves have changed in this PR, as it seems like there hasn't been much design/copy feedback around it. |
talldan
reviewed
Feb 21, 2024
| { | ||
| // Template blocks. | ||
| // The warning is only shown when a user manipulates templates or template parts. | ||
| postTypes: [ 'wp_template', 'wp_template_part' ], |
There was a problem hiding this comment.
Not sure if this message needs to display when editing any other post types than the ones specified here? Or maybe it should be universal (happens for any post type).
talldan
reviewed
Feb 21, 2024
| if ( removedTemplateBlocks.length ) { | ||
| return _n( | ||
| 'Deleting this block will stop your post or page content from displaying on this template. It is not recommended.', | ||
| 'Some of the deleted blocks will stop your post or page content from displaying on this template. It is not recommended.', |
There was a problem hiding this comment.
I revised the copy that was in trunk as we don't know that all of the deleted blocks are dangerous to be removed, it could be four paragraphs and one query loop.
We could also go through all the permutations to make sure the messaging is completely correct, but I thought this would be sufficient.
|
This should be good for review now. |
youknowriad
approved these changes
Feb 21, 2024
andrewserong
approved these changes
Feb 22, 2024
…he editor package move the block specific message wording back into the editor provider so the block editor is not polluted with `template` terminology, and move the pattern specific check into a callback which is passed in via the rules object Move removal warnings setup into own component Fix e2e Another e2e fix Remove the intro text remove select from callback Simplify the API so standard for specific blocks and more generic checks like pattern overrides
- Callbacks now receive all removed blocks and return the removal prompt message - Filter for post type in the component instead of the block editor action
talldan
force-pushed
the
refactor/block-removal-warning
branch
from
625d59d to
b2925d7
Compare
|
Flaky tests detected in dd72cd7. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7999860660
|
|
Thanks for the reviews ❤️ |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Refactors the block deletion warnings
Why?
#58796 Added some code to show a warning if a user tries to delete a block that has pattern overrides, but could do with some code improvements.
This follow up makes the block removal warnings more flexible to cover the use case for patterns
How?
wordpress/editorpackage along with the copy, so that it works across the post and site editor automatically.postTypethat the associated message should appear for. This is mostly use to ensure the message for pattern overrides only shows when editing a pattern (see Patterns: Avoid showing block removal warning when deleting a pattern instance that has overrides)trunk.Testing Instructions
Screenshots or screencast