Merged
Conversation
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; | ||
| } |
Member
Author
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
|
9a70526 to
4156968
Compare
Contributor
|
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. |
3a1b0a8 to
8ae9a7b
Compare
Member
Author
Do you have reproduction steps? I can't seem to reproduce it 🤔 🙇♂️
I forgot it! Thanks! I just added them in 8ae9a7b :) |
Contributor
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). |
Member
Author
|
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. |
8ae9a7b to
81b5a31
Compare
tellthemachines
approved these changes
May 19, 2021
Contributor
tellthemachines
left a comment
There was a problem hiding this comment.
Works well and code looks good, just a few questions below, but ✅
| ] ); | ||
|
|
||
| useEffect( () => { | ||
| return sidebar.subscribeHistory( () => { |
Contributor
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.
Member
Author
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.
Contributor
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.
Member
Author
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 :)
Contributor
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.
| return `widget_${ idBase }`; | ||
| } | ||
|
|
||
| function debounce( leading, callback, timeout ) { |
Contributor
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.
Member
Author
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.
Contributor
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.
Contributor
There was a problem hiding this comment.
Don't we need to bind hasUndo/hasRedo too?
Member
Author
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
Contributor
draganescu
left a comment
There was a problem hiding this comment.
Tested this and it works very well. Let's revisit with polishes later.
81b5a31 to
a84dcb6
Compare
a84dcb6 to
736bdf7
Compare
Contributor
|
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
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 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.
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).