Block Bindings: Output granular post meta changes in a list inside save panel#62982
Block Bindings: Output granular post meta changes in a list inside save panel#62982artemiomorales wants to merge 9 commits intotrunkfrom
Conversation
artemiomorales
added
[Feature] Block bindings
[Type] Enhancement
|
Size Change: +829 B (+0.05%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
This is still a work in progress, though I could use an opinion on the following:
I'll continue exploring this other point:
|
This may work: For the rest of the questions, I still need to re-read everything 😅 |
@cbravobernal Great, thanks! I switched to use the deep comparison, which seems like the right move and resolves my questions regarding the objects. Still need to explore if this approach would work for other the other mentioned issues. |
|
Flaky tests detected in a9b7067. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9730548391
|
I am not sure we should export a function that lives inside a component that is specific to global styles, which seems unrelated to this. The description of that function says "A deep comparison of two objects, optimized for comparing global styles." I'm wondering if we should create our own function for compare or create a global util for this. Doesn't EDIT: Oh, we want to compare and get the diff, not just check equal, right?
I assume it is possible to have an |
Agree with that. It's weird we didn't need it until then. But usually there could be an utils file per package. If it will be widely used in the project, then a |
Right, the diff is why fastDeepEqual isn't good enough for this use case.
Ok will look at this 👍
Ok, I can create a deep equal comparison for the |
artemiomorales
force-pushed
the
update/save-panel-granular-metadata
branch
from
4b01694 to
fd6caf7
Compare
artemiomorales
left a comment
artemiomorales
left a comment
There was a problem hiding this comment.
I think this PR is ready for feedback. Would appreciate any thoughts especially on the below!
| } | ||
| } | ||
| return diffs; | ||
| } |
There was a problem hiding this comment.
How does this deep equality comparison look? The one mentioned in this comment works well, so I essentially copied it, just changing the appearance of the diff slightly. I'm not sure in what other scenarios metadata might change or how, and if this comparison would suit those scenarios.
There was a problem hiding this comment.
Also, since we're only using the deep comparison in this file, I decided to just keep it local rather than creating a utils file for now. How does that sound?
There was a problem hiding this comment.
The only thing that changes is the way we show nested properties, right? (This line)
For me, both cases are really similar so maybe it makes sense to keep consistency between them?
Additionally, I think it's fine to keep it local for now, but I would open a follow-up pull request to move deepCompare to a util to foster the discussion. It seems that at least global styles and post meta could benefit from this. I assume it should be "format agnostic" and it can be formatted after the deepCompare return for each specific use case.
| // Example: isShallowEqualObjects( { a: undefined }, { b: 5 } ) | ||
| ( aValue === undefined && ! b.hasOwnProperty( key ) ) || | ||
| aValue !== b[ key ] | ||
| ! isEqual( aValue, b[ key ] ) |
There was a problem hiding this comment.
In using getPostMetaChanges(), I was receiving a warning: The 'use select' hook returns different values when called with the same state and parameters. This can lead to unnecessary re-renders.
Here's a screenshot of the values being compared along with the warning:
This check is called in the following lines:
gutenberg/packages/data/src/components/use-select/index.js
Lines 161 to 166 in 7dda8fd
Essentially, a new array is returned each time from getPostMetaChanges(), causing the warning even though the array values are equal. What's the best way to handle this?
I'm not sure the equality check was written with this scenario in mind, so I revised it to remove the warning and aid with discussion. Would appreciate any pointers or thoughts 🙏
There was a problem hiding this comment.
I don't feel fully comfortable changing the conditional of the warning. I would like to understand better why the existing implementation is triggering that warning. I've been doing some checks with global styles, which is a really similar use case to what we are trying to achieve and this is what I found:
In trunk
In the Site Editor:
- when I load a specific post, it doesn't trigger any warning.
- when I modify a global style and click save, it doesn't trigger any warning.
In this branch without the changes in is-shallow-equal package
- when I load any post, even if it doesn't have any block connected, it triggers the warning.
- when I modify a global style and click save, it DOES trigger the warning.
- when I modify post meta and click save, it DOES trigger the warning.
That makes me think that something might be wrong with this implementation. That's why I would like to understand better the root cause. Why is it working in trunk for global styles, which is a really similar use case, and not in this case?
There was a problem hiding this comment.
This shouldn't change and the warning tries to protect us from unnecessary rerenders. Probably what we need here is have a selector that gets the data(returns stable reference) and do the comparison in the consumer components.
We could keep the hasPostMetaChanges selector probably and only calculate the diff if it has changes.
After taking a deeper look, I believe these issues are all best handled separately, as their needs and discussions, while related, will be different. |
|
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. |
artemiomorales
requested review from
SantosGuillamot,
cbravobernal and
ntsekouras
artemiomorales
changed the title
| ( postMetaChanges && | ||
| postMetaChanges.length > 0 && |
There was a problem hiding this comment.
Is not enough to use postMetaChanges?.length here?
| * @return {string[]} An array of paths whose values have changed. | ||
| */ | ||
| function deepCompare( changedObject, originalObject, parentPath = '' ) { | ||
| // We have two non-object values to compare. |
There was a problem hiding this comment.
This doesn't seem to be the place to have such a util. It has nothing to do with the store.
|
I'm closing this for now; explorations can continue if or when discussion on the UX regarding post meta changes continues. |
4 participants
What?
This PR granularly outputs post metadata changes in the save panel.
Why?
Begins addressing #62938
We want to give users better visibility on what is being saved when post metadata is modified.
How?
It renames the existing
hasPostMetaChangestogetPostMetaChangsand compares the original post object'smetaproperty to the modified one.Testing Instructions
1. Register some post meta fields by adding this snippet to your theme's functions.php
2. In published post, add paragraph blocks bound to the custom fields using the Code Editor
Override the paragraph contents by typing into the bound paragraphs.
Press Save and see that an indicator for the meta changes is shown in the save panel.
Repeat the same for the heading, image, and button blocks.
Testing Instructions for Keyboard
Screenshots or screencast