Add undo redo to Widgets Customizer #31653
Merged
Conversation
This file contains 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
kevin940726
added
the
[Feature] Widgets Customizer
kevin940726
commented
May 10, 2021
Comment on lines
+175
to
+178
| // Bail out updates if the updated widgets are the same. | ||
| if ( isShallowEqual( sidebar.getWidgets(), nextWidgets ) ) { | ||
| return prevBlocks; | ||
| } |
There was a problem hiding this comment.
This is to prevent changes from being pushed to the history stack when switching from edit form to preview mode in Legacy Widgets. This is probably a bug in the Legacy Widgets block though and we should probably fix it there. @noisysocks, any ideas why it triggers a new change even though the instance is the same?
|
Size Change: +916 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
kevin940726
force-pushed
the
add/undo-redo-widgets-customizer
branch
from
9a70526
to
4156968
Compare
|
This is working great, @kevin940726 🎉 I tried out a variety of blocks and legacy widgets, and the undo/redo actions seem to be working as expected based on the behavior in the post editor. A couple of minor notes: The undo action doesn’t work great on some legacy widgets like SiteOrigin widgets but I'm not sure that this is a blocker. (In an ideal world, I think you’d be able to step back through each of the changes made to input forms while in editing mode). Keyboard commands ⌘Z and ⇧⌘Z are not working for me but I’m assuming those just haven’t been added yet. |
kevin940726
force-pushed
the
add/undo-redo-widgets-customizer
branch
from
3a1b0a8
to
8ae9a7b
Compare
Do you have reproduction steps? I can't seem to reproduce it 🤔 🙇♂️
I forgot it! Thanks! I just added them in 8ae9a7b :) |
Nice!
For example, if you have a SiteOrigin legacy widget and change the text in one of the input forms while editing, the Undo action doesn't seems to work (if you try to change and undo the "Title text" for the SiteOrigin Image widget, for example). It does seem to work with some other legacy widgets (ex. Astra: Social Profiles). |
|
Hmm yeah it seems like some legacy widgets have problems with undo/redo, even when it's in the widgets screen. Let's tackle it in a new PR though, as I believe the problem is in the legacy widgets. |
kevin940726
force-pushed
the
add/undo-redo-widgets-customizer
branch
from
8ae9a7b
to
81b5a31
Compare
tellthemachines
approved these changes
May 19, 2021
| ] ); | ||
|
|
||
| useEffect( () => { | ||
| return sidebar.subscribeHistory( () => { |
There was a problem hiding this comment.
Not sure I get why we're subscribing in the effect cleanup as opposed to in the effect itself.
There was a problem hiding this comment.
Not sure I understand? We are subscribing in the effect itself, but also returning the unsubscribe callback to the cleanup callback. It's a common pattern for event listeners.
There was a problem hiding this comment.
What confused me is it's not clear at a glance what sidebar.subscribeHistory does. the usual pattern for effects is something like:
useEffect( () => {
subscribe()
return () => unsubscribe()
})
but in this case, both the subscribe() and the returning of the cleanup function are happening in sidebar.subscribeHistory.
There was a problem hiding this comment.
I'd argue that this pattern is in fact very common. As both Redux's store.subscribe and history's listen are using this pattern to return an unsubscribe function.
However, I agree that it never hurts to make it more clear. I'll try to keep that in mind next time :)
There was a problem hiding this comment.
Yeah, after digging in a bit I realised it occurred in other places. Was just surprising the first time 😅
I guess depending on how complex the logic is in the effect it might help to add a comment, but in this case it's pretty straightforward.
| @@ -27,6 +27,32 @@ function widgetIdToSettingId( widgetId ) { | |||
| return `widget_${ idBase }`; | |||
| } | |||
|
|
|||
| function debounce( leading, callback, timeout ) { | |||
There was a problem hiding this comment.
Do we need to implement debounce from scratch? Elsewhere we're using lodash, and there's also a useDebounce (which seems to be a wrapper for lodash's debounce) in our compose package.
There was a problem hiding this comment.
I'm afraid we have to. We want to call different debounce callbacks depending on whether it's the leading call or not. I can't find a way of doing so with lodash's debounce or any other implementations.
There was a problem hiding this comment.
Could be worth adding a comment to that effect, this'd be something someone with the best intentions might accidentally replace with the lodash function.
|
|
||
| this.setting.bind( this._handleSettingChange.bind( this ) ); | ||
| this.api.bind( 'change', this._handleAllSettingsChange.bind( this ) ); | ||
|
|
||
| this.canUndo = this.canUndo.bind( this ); | ||
| this.canRedo = this.canRedo.bind( this ); |
There was a problem hiding this comment.
Don't we need to bind hasUndo/hasRedo too?
There was a problem hiding this comment.
Only if we're destructing it from the sidebar instance. Currently we're calling it directly so that we can keep referencing this to the instance.
draganescu
approved these changes
May 19, 2021
kevin940726
force-pushed
the
add/undo-redo-widgets-customizer
branch
from
81b5a31
to
a84dcb6
Compare
talldan
force-pushed
the
add/undo-redo-widgets-customizer
branch
from
a84dcb6
to
736bdf7
Compare
|
I've rebased. Hopefully tests pass. 🤞 |
jffng
pushed a commit
that referenced
this pull request
May 19, 2021
* Add undo redo and debounce * Style the undo redo button * Bail out updates if the next widgets are the same * Add debounce * Add missing keyboard shortcuts * Fix styling
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description
Close #30400. Probably need a rebase after #31589.
Implement a naive solution for undo redo in the Widgets Customizer. The changes are debounced for 1 second to micmic the same behavior in the block editor.
How has this been tested?
Manually. e2e tests TBD.
Screenshots
Kapture.2021-05-10.at.17.12.57.mp4
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).