-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 Editor: Mark last change as persistent on save #36887
Block Editor: Mark last change as persistent on save #36887
Conversation
cc @youknowriad. |
@@ -159,6 +164,8 @@ export default function EntitiesSavedStates( { close } ) { | |||
siteItemsToSave | |||
); | |||
} | |||
|
|||
__unstableMarkLastChangeAsPersistent(); |
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.
Maybe instead of dispatching this here, we could do so inside the close
callback so we don't dispatch this action on every instance of EntitiesSavedState
.
But on the other hand, it makes sense to dispatch this action here since it's part of editing save.
We might even do some refactoring removing other
Only on the site editor. I could not replicate the behavior on the post editor. |
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 looks reasonable to me. It sidesteps a lot of issues with our first approach, and it gets the save feature working again for block settings attributes.
+1 from me, but we should wait for Ella or Miguel to chime in. Like Riad mentioned, they have more experience with and levels and dirty checking.
It's almost a week without additional feedback, so we're assuming the solution is good enough. This is affecting FSE users. We’re aiming to merge this really early tomorrow, but wanted to leave a comment so that it doesn’t take anyone by surprise. cc @youknowriad, @ellatrix, @ntwb. |
I apologise for not commenting earlier, but it took me a while to dig into the problem.
I think the post-editor question is crucial, and we should be able to answer it with confidence. It's an important clue that gutenberg/packages/editor/src/store/actions.js Lines 271 to 278 in e96d47b
The Site Editor doesn't have an equivalent action. In fact, the To get back to the point of this PR, I was initially suspicious of tucking away such important behaviour in a UI component like I think that, in the future, we should keep all this delicate logic in some place that's central to the whole Site Editor to avoid bad integrity bugs, race conditions, etc. But, right now, I think this PR will do. One last thing: please thoroughly test saving, editing, undoing not just in the Site Editor, but in the Post Editor too — I don't expect overlap based on this PR's changes, but just to be safe. |
Thanks for digging into this @mcsf! Much appreciated.
I'm looking into everything you described now. I'll be sure to leave detailed testing notes of my findings. 👍 |
TestingPost Editor Testing Steps + Requirements
Undo and Redo
Save
Edit
Site Editor Testing Steps + Requirements
Undo and Redo
Save
Edit
Browsers
NotesThere are a variety of existing undo and redo bugs on Gutenberg trunk.
|
@mcsf The undo/redo functionality appears to be in a rough state on its own in I've catalogued any bugs that I've found in |
* Mark last change as persistent when saving changes
Description
Fixes #36417.
It makes sense to check the last change as persistent once the "Save" button is clicked, even if the attributes themselves didn't change. Not doing so results in the save button not being enabled if editing the same attribute after clicking "Save" once.
Let me know if there's a better way of performing such action or if I'm missing something.
How has this been tested?
Screenshots
Before
After
Types of changes
Bug fix
Checklist:
*.native.js
files for terms that need renaming or removal).