Add indicator for metadata changes to Save Panel when reviewing modified entities #61811
Conversation
artemiomorales
changed the base branch from
trunk
to
try/allow-editing-in-post-meta-bindings
artemiomorales
changed the title
|
With this PR, so far I've aimed to just explore the relevant areas we'd likely need to modify to get this functionality working. Some caveats with it right now:
One takeaway for me is that we'll likely need to do some refactoring around the Also, as far as i can see, all of the metadata is present in the post entity's Anyway, potentially this can be a starting point as we look to get this working. Will keep looking at it. |
|
Size Change: +2.6 kB (+0.15%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
SantosGuillamot
force-pushed
the
try/allow-editing-in-post-meta-bindings
branch
from
995607a
to
b3f4488
Compare
|
Nice work. Despite following the steps I wasn't able to make the paragraph editable, so I wasn't able to test this myself. That said, from the screenshot, this seems like a very useful disclosure for people who are using this feature, and an important addon to any other affordances we make. Note I did see some errors with the site editor loading, as part of this draft, just in case that's not on my end. These are all grouped under "Posts", yet all of these are metadata for one post, correct? If yes, then we should group them under under each page item. In the future you could see multiple pages edited in one session, and published together. The following explores collapsibles, icons, separators, I'd love some @WordPress/gutenberg-design input on those pieces. 
Figma, if inspiration strikes. But the main challenge to solve is to show that all those checkboxes as being related to the particular post, I.e. if you edit multiple posts, or more likely, pages in the site editor, and their bound values, you'd see a list of edited pages. So we need to mainly get that separation right. |
That was my initial reaction upon seeing the mockup. IE shouldn't each field appear within the associated post? |
|
Had a good chat with Artemio around what's feasible for next Friday. So I'd like to clarify a bit what I perceive as important here, and that is, ultimately: to make it clear to a user who's editing bound blocks, that they've done so. The established flow we have for surfacing this, is the pre-publish panel. Atomically breaking out every single property, adding icons and collapsibles and indentation as I showed above, that can be a future refinement. The important piece here is the clarity of surfacing: hey, you edited bound blocks, are you sure? And that can be as little as this: 
To me that seems a fine V1 to go with. |
artemiomorales
force-pushed
the
try/bindings-add-entities-panel
branch
from
4b9bbd7
to
db7862c
Compare
SantosGuillamot
force-pushed
the
try/allow-editing-in-post-meta-bindings
branch
2 times, most recently
from
5fdbacb
to
8e6dd67
Compare
artemiomorales
force-pushed
the
try/bindings-add-entities-panel
branch
from
462c1bc
to
b9e7cde
Compare
artemiomorales
changed the base branch from
try/allow-editing-in-post-meta-bindings
to
try/sync-block-context-in-store
artemiomorales
force-pushed
the
try/bindings-add-entities-panel
branch
from
144505c
to
195a32d
Compare
|
@jasmussen @SantosGuillamot Ok this is what I've been able to get so far: 
I haven't been able to get a checkbox working to granularly handle the editing of the block bindings. I'll keep working on it, but I'm not sure if we'll be able to get that before the WP 6.6 beta.
Note that last I checked, the latest branch for editing the meta fields doesn't work independently, so this PR still has commits from the old version of that branch, which means the code isn't ready to be reviewed, but the UX can be looked at. |
|
The added presence of these items in pre-publish, even if you can't uncheck the bindings and omit them separately, is a crucial part of the flow for awareness of bindings. Which is especially important as the block-toolbar/in-canvas UI has been a trickier piece to get right and might need more time in the oven. And with the deadline looming, we should start merging. So a 👍 👍 on the design aspect, even if there are some spacings, margins, and line heights we need to follow up with. |
packages/core-data/src/selectors.ts
Outdated
| @@ -686,6 +718,9 @@ export const __experimentalGetDirtyEntityRecords = createSelector( | |||
| entityConfig?.getTitle?.( entityRecord ) || '', | |||
| name, | |||
| kind, | |||
| hasMetaChanges: metadataKeys.includes( primaryKey ) | |||
There was a problem hiding this comment.
Looking at the existing logic, it feels like all the changes required in this selector could get moved to this line. I'm thinking about something like the following:
hasMetaChanges: !! getEntityRecordNonTransientEdits(
state,
kind,
name,
primaryKey
).?meta,All the conditions should be satisfied because these operations happen only for primaryKeys computed in the earlier stage.
There was a problem hiding this comment.
A related question is whether in the long run, the boolean property is going to be sufficient, or it would be preferred to list all individual modified meta changes. I was about to say that hasMetaChanges will become part of the public API, but I see that it's prefixed as experimental so it is less important. Anyway, it's good to answer the question if the boolean value can serve its purpose in the far future. Maybe, it's fine to compute all that locally instead by using getEntityRecordNonTransientEdits when it's clear that a given entity has some edits applied.
There was a problem hiding this comment.
I was gonna share a similar question. I was wondering if we should abstract it to not include only metaChanges but track the different changes made to the post and do with that information whatever we consider.
I'm thinking that in the future we will probably have the possibility to edit "Post Data" like title, description, excerpt... And we might want to do something similar.
There was a problem hiding this comment.
Anyway, it's good to answer the question if the boolean value can serve its purpose in the far future.
I was gonna share a similar question. I was wondering if we should abstract it to not include only
metaChangesbut track the different changes made to the post and do with that information whatever we consider.
Yes I think we'd ideally have a granular listing of all the meta fields that have changed, which means a simple boolean would not be enough in the future, though the boolean is a good starting point.
There was a problem hiding this comment.
I followed @gziolo's advice and simplified the logic 👍
| @@ -125,7 +125,8 @@ export const hasNonPostEntityChanges = createRegistrySelector( | |||
| ( entityRecord ) => | |||
| entityRecord.kind !== 'postType' || | |||
| entityRecord.name !== type || | |||
| entityRecord.key !== id | |||
| entityRecord.key !== id || | |||
| entityRecord.hasMetaChanges | |||
There was a problem hiding this comment.
I am not sure if the hasNonPostEntityChanges is the proper place to do this. Meta changes are actually part of the post entity, so I find it a bit confusing. I don't know where the selector is used but maybe we need a new one or rename it?
There was a problem hiding this comment.
Ok I introduced a new function called hasMetaChanges in order to make this check independent.
| <PanelRow> | ||
| <Flex className="entities-saved-states__block-bindings"> | ||
| <Icon | ||
| className="entities-saved-states__connections-icon" | ||
| icon={ connection } | ||
| size={ 24 } | ||
| /> | ||
| <span className="entities-saved-states__bindings-text"> | ||
| { __( 'Block Bindings.' ) } | ||
| </span> | ||
| </Flex> | ||
| </PanelRow> |
There was a problem hiding this comment.
I don't know if the message we show should be "Block Bindings". To me, it seems related to just Post Meta, not Block Bindings. If I am not mistaken, any change made to custom fields (with or without block bindings) will trigger this message, right?
There was a problem hiding this comment.
I had the same observation. I don't think we track, nor should we even try to do so, whether an edit was triggered by the established Block Binding. It's more about bringing more detailed information about what has been edited, similar to when folks edit the site settings:
There was a problem hiding this comment.
So should we rename the message to "Post Meta" and remove the icon? cc @jasmussen
There was a problem hiding this comment.
Maybe, we should flex here to meet the beta deadline to be sure. I do like the icon, and I do think it's important to have some indentation, or space below each post+post-meta chunk, so it's visibly clear that "post meta" is not some single unit you can save globally, it's attached and related to each post.
There was a problem hiding this comment.
The tricky thing with 'Post meta' is that the scope is wider than 'Bindings'.
For instance why would a 'Post meta' item be surfaced in the save panel when a block binding is changed, but not when something like the slug, featured image, or discussion settings are changed?
artemiomorales
force-pushed
the
try/bindings-add-entities-panel
branch
from
195a32d
to
00c9e1f
Compare
artemiomorales
changed the base branch from
try/sync-block-context-in-store
to
trunk
|
This pull request changed or added PHP files in previous commits, but none have been detected in the latest commit. Thank you! ❤️ |
|
I rebased on With that in mind, it's possible to test this using the store in If using the console to debug, the best bet is to modify existing content in a post, then use the following commands in the console, though you may need to do this twice before it works, likely due to some inconsistency in updating via code versus the UI. |
|
Flaky tests detected in 23d9429. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9318524438
|
artemiomorales
requested review from
nerrad,
ajitbohra and
ryanwelcher
as code owners
|
@SantosGuillamot @gziolo Is circumventing the footnotes a blocker for this PR? |
| import type { Editor } from './index'; | ||
|
|
||
| /** | ||
| * Clicks on the button in the header which opens Document Settings sidebar when |
There was a problem hiding this comment.
I don't think this description is correct.
There was a problem hiding this comment.
I'm wondering if it is okay to create a new global util or start with something specific for this tests. Something like the UndoUtils, for example. (There are many other tests with their own utils)
There was a problem hiding this comment.
I updated the description and moved the test 👍
In my opinion, I would like to avoid interfering with the current workflows. I'll take a look to see if I can help somehow. |
I left my observations from testing this PR in #61811 (comment). I don't think that the way footnotes block is connected to the post meta is a blocker alone. The way I see it, we need to have a good strategy for when to interfere with the publishing flow and show proper information. The way I would try to approach the initial version would be:
|
|
Regarding adding a workaround for export const hasPostMetaChanges = createRegistrySelector(
( select ) => ( state, postType, postId ) => {
const { type: currentPostType, id: currentPostId } =
getCurrentPost( state );
// If no postType or postId is passed, use the current post.
const edits = select( coreStore ).getEntityRecordNonTransientEdits(
'postType',
postType || currentPostType,
postId || currentPostId
);
if ( ! edits?.meta ) {
return false;
}
// Compare if anything apart from `footnotes` has changed.
const originalPostMeta = select( coreStore ).getEntityRecord(
'postType',
postType || currentPostType,
postId || currentPostId
)?.meta;
delete originalPostMeta[ 'footnotes' ];
delete edits.meta[ 'footnotes' ];
return ! fastDeepEqual( originalPostMeta, edits.meta );
}
);
Do you think something like that could make sense? I tested and it seems to work. |
|
Okay, so what is needed to merge this pull request? Some thoughts I have in my mind: Testing
Remaining tasks/decisionsThese are some tasks/decisions that could be relevant:
|
Regarding this, it seems to be happening the same for other selectors like |
I'd say that shouldn't be a blocker for this PR, and we can iterate if need be during the beta period. |
|
I've tested the following scenarios from @SantosGuillamot's comment and @gziolo's comment, and it all seems to work as expected:
Regarding the following point, we've added a workaround at least for footnotes thanks to @SantosGuillamot:
Regarding metadata besides footnotes, the panel appears intentionally when that metadata has changed; there may be other cases similar to footnotes where we don't want to show the save panel, but I believe we can add any additional exceptions during the beta period. The behavior in the site editor regarding different combinations of modifying metadata for posts also seems to be working as expected as per the third bullet point above. I've also moved the location of the end-to-end test utils around modifying metadata. That means that the only outstanding points are the following, which I believe will require more investigation and/or discussion to resolve:
I wouldn't consider either of those blockers though and believe this PR is ready to be merged. Thoughts? |
This reverts commit 2b509ce.
I created an issue for this bug: #62236 |
…ied entities (WordPress#61811) * Add rough prototype showing meta in entities panel * Simplify detection of metadata changes Removed unnecessary code added in previous commit and instead modified existing functions to add a flag on existing dirtyEntityRecord structures to indicate when metadata changes have been made. * Remove obsolete code * Add indicator for bindings to save entities panel * Modify message to read 'Post Meta' * Add store function to check if meta has changed * Remove obsolete check * Simplify logic to check if meta has changed * Update tests * Make hasMetaChanges selector private * Suggestion: Move logic to `hasPostMetaChanges` selector * Change test formatting * Don't show save panel in pre-publish * Get `hasPostMetaChanges` from the proper place * Add end-to-end test * Update class name * Clarify naming * Show Post Meta in relevant post * Remove extra change * Move test metadata test util * Update comments * Prevent save panel from appearing when just footnotes are modified * Update package-lock.json --------- Co-authored-by: Mario Santos <santosguillamot@gmail.com>
…ied entities (WordPress#61811) * Add rough prototype showing meta in entities panel * Simplify detection of metadata changes Removed unnecessary code added in previous commit and instead modified existing functions to add a flag on existing dirtyEntityRecord structures to indicate when metadata changes have been made. * Remove obsolete code * Add indicator for bindings to save entities panel * Modify message to read 'Post Meta' * Add store function to check if meta has changed * Remove obsolete check * Simplify logic to check if meta has changed * Update tests * Make hasMetaChanges selector private * Suggestion: Move logic to `hasPostMetaChanges` selector * Change test formatting * Don't show save panel in pre-publish * Get `hasPostMetaChanges` from the proper place * Add end-to-end test * Update class name * Clarify naming * Show Post Meta in relevant post * Remove extra change * Move test metadata test util * Update comments * Prevent save panel from appearing when just footnotes are modified * Update package-lock.json --------- Co-authored-by: Mario Santos <santosguillamot@gmail.com>
What?
This is a first iteration of adding metadata to the Save Panel when reviewing modified entities.
Why?
Begins addressing #61405
Related to #61753
We want to indicate to the user when they are modifying the value of custom fields.
How?
Overrides existing logic to determine whether to show the Save Panel, while also showing that metadata has changed.
Testing Instructions
1. Register post meta by adding this snippet to your theme's functions.php
2. Add a paragraph block bound to the custom field using the Code Editor
Testing Instructions for Keyboard
Screenshots or screencast