You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
#35892 introduced some changes to the Site Logo block, including an effect which was intended to discard any unsaved changes to the logo/icon if a user removes all their Site Logo blocks. This effect relied on getGlobalBlockCount to determine the existence of any Site Logo blocks, but does not work reliably in the Site Editor. If it fires erroneously, it breaks the Redo button. (See testing instructions).
This PR just removes that logic entirely in order to fix the Redo bug. Other options for discarding unsaved Site Logo changes can be explored separately at a much lower priority.
How has this been tested?
Verify you can reproduce the bug on trunk:
Load the Site Editor.
In the header template part, insert a Site Logo block and update the image.
Click into a different template part, which does not contain a Site Logo block. Insert any other block.
Click the "undo" button
Try to "redo" and notice the action is unavailable.
Now checkout this branch and repeat the testing instructions. This time, the "Redo" should be available and should re-insert the block removed by "Undo".
Screenshots
Types of changes
Checklist:
My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
I've tested my changes with keyboard and screen readers.
My code has proper inline documentation.
I've included developer documentation if appropriate.
I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).
This effect ran on unmount and used `getGlobalBlockCount` to determine
if all Site Logo blocks have been removed, then discards any unsaved
changes to the Logo and Icon.
This does not work correctly in the Site Editor context, and would fire
erroneously when switching between template parts, breaking the ability
to 'Undo'/'Redo'.
The reason will be displayed to describe this comment to others. Learn more.
Thanks for putting together this fix @stacimc! I had a little trouble replicating the issue at first (it seemed like the first time I do undo/redo it worked on trunk but if I do undo/redo a second time after removing all site logo blocks, I could replicate it — or, if I edit outside of a template part I could replicate it more reliably).
Before: redo stops working
Kapture.2022-01-12.at.11.23.13.mp4
After: redo works reliably
Kapture.2022-01-12.at.11.27.58.mp4
This removal looks good to me to restore the redo behaviour in the site editor. 🤞 we can come up with another idea for how to preserve discarding changes when removing the last site logo, but that sounds like a lower priority task than getting this fix in. LGTM!
The reason will be displayed to describe this comment to others. Learn more.
Tested well for me.
Would be nice to look for another solution for removing site logo block updates when all blocks are removed, but it makes sense to fix the regression by removing this code first
…37895)
This effect ran on unmount and used `getGlobalBlockCount` to determine
if all Site Logo blocks have been removed, then discards any unsaved
changes to the Logo and Icon.
This does not work correctly in the Site Editor context, and would fire
erroneously when switching between template parts, breaking the ability
to 'Undo'/'Redo'.
[Feature] HistoryHistory, undo, redo, revisions, autosave.[Feature] Site EditorRelated to the overarching Site Editor (formerly "full site editing")[Type] BugAn existing feature does not function as intended
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #37760
Description
#35892 introduced some changes to the Site Logo block, including an effect which was intended to discard any unsaved changes to the logo/icon if a user removes all their Site Logo blocks. This effect relied on
getGlobalBlockCount
to determine the existence of any Site Logo blocks, but does not work reliably in the Site Editor. If it fires erroneously, it breaks theRedo
button. (See testing instructions).This PR just removes that logic entirely in order to fix the Redo bug. Other options for discarding unsaved Site Logo changes can be explored separately at a much lower priority.
How has this been tested?
Verify you can reproduce the bug on trunk:
Now checkout this branch and repeat the testing instructions. This time, the "Redo" should be available and should re-insert the block removed by "Undo".
Screenshots
Types of changes
Checklist:
*.native.js
files for terms that need renaming or removal).