-
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: Output granular post meta changes in a list inside save panel #62982
Conversation
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: gutenberg/packages/block-editor/src/components/global-styles/get-global-styles-changes.js Line 71 in 15da2aa
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 |
4b01694
to
fd6caf7
Compare
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -31,7 +57,7 @@ export default function isShallowEqualObjects( a, b ) { | |||
// | |||
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
if ( ! isShallowEqual( mapResult, secondMapResult ) ) { | |
// eslint-disable-next-line no-console | |
console.warn( | |
`The 'useSelect' hook returns different values when called with the same state and parameters. This can lead to unnecessary rerenders.` | |
); | |
didWarnUnstableReference = true; |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
( postMetaChanges && | ||
postMetaChanges.length > 0 && |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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
hasPostMetaChanges
togetPostMetaChangs
and compares the original post object'smeta
property 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