-
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
Block Bindings: Remove "Site Updated" notification for post entity changes #63223
Conversation
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.
As noted in the comment below, I'm not sure if this fix works in all scenarios or is the best solution. Let me know what you think 🙏
|
||
// This flag allows us to avoid showing the 'site updated' message | ||
// in scenarios related to entity changes when the message shouldn't be | ||
// triggered, like when modifying post meta via block bindings. | ||
if ( kind !== 'postType' ) { | ||
showSiteUpdatedMessage = true; | ||
} |
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.
Previously, the logic in this code assumed that an entity change must be a site change; this just introduces a flag to circumvent the Site Updated
message if the entity change was of kind postType
.
I think it works, but I’m not sure this is the most robust solution.
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.5 kB (-0.09%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
Before reviewing the code, I have a conceptual question: In which scenarios do we want to show the "Site updated" message, and which others should we show "Post updated" or other things? For example, if I'm on a page with a query loop and modify the custom fields of other posts, which message should appear? |
I just came across this comment from Riad: link.
|
@youknowriad Do you think we should still take this approach? It looks like the "Template Updated" message is handled somewhere else. Maybe checking explicitly for 'postType' as in this PR at the moment is good enough? I'll explore doing the smarter option too, but wanted to get your thoughts on this. |
Thanks for taking another stab at this, I like that that it takes a more generic approach but I noticed a few things: We basically have two different save flows that result in different logic for the notices:
1- I think ideally the "save process" notification handling (maybe even the save action itself but this is out of scope) should be the same. It would be good to use the exact same util for both. Whether that means making 2- It's still unclear what the message should be for me in the case of multiple entities. What happens when I save just the "site" entity, what happens when I save "template" and "post", what happens when I save "template and template part", what happens when I save "site" and "post"... I think some help from designers mapping out the different cases would help us implement the right solution. cc @jameskoster @jasmussen 3- While testing this PR, I noticed random things like when saving "template" and "site", I end up with two notices : "Template updated" and "post updated", why? |
For single-saves, my take is that what we have works reasonably well:
I wonder if we should expand that to cover site-specific data, e.g:
Or whether a singular "Site updated" is adequate. No strong opinion there. For multi-entity, to differentiate from the 'site' item, perhaps a more generic "All changes saved" could work? |
Ok, thanks for the guidance! I can try refactoring the
I can change the multi-entity save notification to be "All changes saved."
My initial reaction is to keep the first 3 (post/pages, template, and pattern/template part) as specified, and just use "Site updated" for other changes. I'll aim to get something working so we can take a look. |
Sharing same comment as in the issue: The changes to trigger the save panel after editing post meta in a post have been reverted in this pull request. I guess this is not an issue anymore and we just have to ensure that whatever we build doesn't trigger this again. I've opened this new issue to gather all the context and make decisions before working on a new solution. |
I'm going to close this for now, and we can reopen this or explore further whenever we discussions continue around notifications for post entity changes. Here's a related comment regarding similar ideas around post entity changes. |
What?
Currently, the block editor erroneously shows a
Site Updated
notification when modifying a block connected to post metadata via block bindings as reported in #62236. This PR fixes that.Why?
We don't want to show users misleading or incorrect information.
How?
It introduces a check to see if the current entity changes are of the kind
postType
, and it only displays theSite Updated
message if they are not.Testing Instructions
1. Register post meta by adding this snippet to your theme's functions.php
2. In the post editor, add a paragraph block bound to the custom field using the Code Editor
Site Updated
message does not appear, as expected.Please also test other scenarios in the site editor to ensure the
Site Updated
message continues to show as normal when expected.Screenshots or screencast
Before
After