Conversation
…n block with override bindings
glendaviesnz
added
[Type] Enhancement
|
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. |
|
Size Change: +1.6 kB (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
glendaviesnz
commented
Feb 8, 2024
packages/block-editor/src/components/block-removal-warning-modal/index.js
Outdated
Show resolved
Hide resolved
glendaviesnz
commented
Feb 8, 2024
Comment on lines
+145
to
+149
| if ( | ||
| blockAttributes?.metadata?.bindings && | ||
| JSON.stringify( | ||
| blockAttributes.metadata.bindings | ||
| ).includes( 'core/pattern-overrides' ) |
Contributor
Author
There was a problem hiding this comment.
because core/pattern-overrides is nested under a range of different bindings, eg. content, url, etc. this seemed like the easiest way to check without needing to iterate through them all.
Contributor
There was a problem hiding this comment.
Yeah, it's not easy, but I think there are some utility functions in the pattern block's edit file that do this check.
Contributor
There was a problem hiding this comment.
Why do have all these block specific logic in place? (not specific to this PR but I feel like we take too much shortcuts and hacks: filters, special cases... we should try to do a bit better and take some time to figure out the right APIs...)
talldan
reviewed
Feb 9, 2024
Comment on lines
+44
to
+56
| const message = | ||
| messageType === 'templates' | ||
| ? _n( | ||
| 'Deleting this block will stop your post or page content from displaying on this template. It is not recommended.', | ||
| 'Deleting these blocks will stop your post or page content from displaying on this template. It is not recommended.', | ||
| blockNamesForPrompt.length | ||
| ) | ||
| : _n( | ||
| 'Deleting this block could break patterns on your site that have content linked to it. Are you sure you want to delete it?', | ||
| 'Deleting these blocks could break patterns on your site that have content linked to them. Are you sure you want to delete them?', | ||
| blockNamesForPrompt.length | ||
| ); | ||
|
|
Contributor
There was a problem hiding this comment.
This is not something that will be scalable if there are more cases for removal warnings in the future.
Maybe the main message should be part of the rule. It might require a bit of revamping of how the rules work (maybe dividing them up into groups like 'templates' and 'pattern-overrides').
Contributor
Author
There was a problem hiding this comment.
Agreed, but my thinking here was that given this is a private API with currently only 2 known use cases, and this close to the 6.5 release, that this was not the right time to look at revamping the rules API. When/if the 3rd use case arises, or when the API is made public is probably a better time to revisit this API to make it more scalable.
Comment on lines
+145
to
+149
| if ( | ||
| blockAttributes?.metadata?.bindings && | ||
| JSON.stringify( | ||
| blockAttributes.metadata.bindings | ||
| ).includes( 'core/pattern-overrides' ) |
Contributor
There was a problem hiding this comment.
Yeah, it's not easy, but I think there are some utility functions in the pattern block's edit file that do this check.
Comment on lines
+142
to
+154
| if ( rules[ 'bindings/core/pattern-overrides' ] ) { | ||
| const blockAttributes = | ||
| select.getBlockAttributes( clientId ); | ||
| if ( | ||
| blockAttributes?.metadata?.bindings && | ||
| JSON.stringify( | ||
| blockAttributes.metadata.bindings | ||
| ).includes( 'core/pattern-overrides' ) | ||
| ) { | ||
| blockNamesForPrompt.add( blockName ); | ||
| messageType = 'patternOverrides'; | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
This is another part that will be a scalability issue.
It'd be interesting to look at ways of improving it, perhaps using callbacks in the rules to check for validity or determine the message being displayed. 🤔
talldan
reviewed
Feb 9, 2024
Comment on lines
+51
to
+55
| : _n( | ||
| 'Deleting this block could break patterns on your site that have content linked to it. Are you sure you want to delete it?', | ||
| 'Deleting these blocks could break patterns on your site that have content linked to them. Are you sure you want to delete them?', | ||
| blockNamesForPrompt.length | ||
| ); |
Contributor
There was a problem hiding this comment.
I think it could mention something like "This block allows instance overrides, deleting it ...", just so that the language used in the interface is consistent.
talldan
approved these changes
Feb 9, 2024
Contributor
talldan
left a comment
talldan
left a comment
There was a problem hiding this comment.
This works well from a user perspective, so I think I lean towards shipping it, though there might be some things to improve in follow-ups.
talldan
deleted the
add/warning-to-deletion-of-overridden-pattern-blocks
branch
youknowriad
reviewed
Feb 12, 2024
| } | ||
|
|
||
| const message = | ||
| messageType === 'templates' |
Contributor
There was a problem hiding this comment.
It seems weird that a message mentioning "templates" which is a WP specific thing lives in the "block-editor" package? Can you clarify the reasoning?
Contributor
Author
There was a problem hiding this comment.
I was led astray by the template wording that was already added previously, and didn't make the connection that the block-editor should not be aware of templates. Will take another stab at this with this in mind.
youknowriad
reviewed
Feb 12, 2024
| import useNavigateToEntityRecord from './hooks/use-navigate-to-entity-record'; | ||
|
|
||
| const { ExperimentalEditorProvider } = unlock( editorPrivateApis ); | ||
| const { BlockRemovalWarningModal } = unlock( blockEditorPrivateApis ); |
Contributor
There was a problem hiding this comment.
Instead of duplicating this logic between edit-post and edit-site, why not add it to the EditorProvider instead?
youknowriad
reviewed
Feb 12, 2024
| // confirmation. | ||
| const blockRemovalRules = { | ||
| 'bindings/core/pattern-overrides': __( | ||
| 'Blocks from synced patterns that can have overriden content.' |
Contributor
There was a problem hiding this comment.
Are these messages that are specific per block type? Should this be a block API instead?
Contributor
There was a problem hiding this comment.
I don't think they are, pattern overrides could in the future potentially be on any block, so it wouldn't make sense to add the rule to every individual block. Unless it was a default part of blocks that support it (it's undecided right now how determining support will work, as it's dictated by the blocks that support bindings), but I think even then that might be a bit much.
I think it could be better as something that can be declared more centrally, but this API does need to be different
to where it landed here IMO.
Contributor
Author
|
See #58952 for an attempt to refactor the |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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?
Adds a confirmation dialog when a user tries to delete a synced pattern with overrides.
Why?
It is currently easy for a user to remove a block from a synced pattern without realising that it was break instances of the pattern that have overridden that content.
How?
Extends the existing
BlockRemovalWarningModalTesting Instructions
Deleteand make sure block is deletedScreenshots or screencast
delete-warning.mp4