-
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
Add undo-redo UI to edit-site and edit-widgets #21955
Add undo-redo UI to edit-site and edit-widgets #21955
Conversation
Size Change: +1.13 kB (0%) Total Size: 820 kB
ℹ️ View Unchanged
|
import { undo as undoIcon } from '@wordpress/icons'; | ||
import { displayShortcut } from '@wordpress/keycodes'; | ||
|
||
export default function UndoButton() { |
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 there a package these components could go in so we aren't duplicating code between edit post, edit site, and edit widgets?
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 block-editor
or alternatively interface
?
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 wouldn't bother sharing these for the moment personally.
I'm also testing undo in #21368, but I've been trying it by inserting template parts in the post editor. My PR is mostly about the dirty state, but it changes how some of the blocks get in and out of the block editor package (and how they relate to their entities), so we have been looking for regressions. I'm "glad" so see that the behavior is a bit buggy despite my code 😁 |
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 is looking good so far. 🎉
Just a minor comment/suggestion below, it looks like its using the hasUndo selector in the redo components.
b56aae1
to
87823dd
Compare
Hi @noahtallen, @Addison-Stavlo thank you for the comments, review, and suggestions the changes were applied 👍 |
Currently the tests are failing with |
The undo buttons seem to be working ok in edit site, but for some reason cmd-z isn't undoing the changes. It's actually weird, so it might be on my side:
It happened for me in brave browser and in firefox dev edition, so not sure if it's just my environment, or if it's something about the shortcuts |
Confirming this as well. The buttons work but the keyboard shortcuts don't seem to. Perhaps this needs to be configured for edit-site somewhere? 🤔 Maybe related? It looks like we don't have something like these set up in site editor yet:
gutenberg/packages/edit-post/src/editor.js Lines 128 to 130 in e110119
So may be related to that? |
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.
It's working for me now, thanks! I still feel like it's weird to be duplicating nearly identical code across several packages, but I don't think it should be a blocker for starting to have this functionality and fixing the more important issues with undo & e.g. template parts :)
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.
Thanks for adding the keyboard shortcuts! This seems to be working for me as well.
For the site editor I guess the best way to test undo/redo is with the site title, as the functionality with child entities is still a bit strange and being fleshed out (although thats a different issue)
In widgets, it seems the undo/redo only works for blocks that are added and not the forms themselves. But I don't know what goals are there.
For the scope of this PR, this seems to be working well as far as I can tell. Only a small nit on naming (if that happens to be any issue there).
const { registerShortcut } = useDispatch( 'core/keyboard-shortcuts' ); | ||
useEffect( () => { | ||
registerShortcut( { | ||
name: 'core/editor/undo', |
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 edit-post it looks like we are using naming convention of core/edit-post/undo
, should these be core/edit-site/undo
/ similarly for widgets?
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.
Hi @Addison-Stavlo nice catch, I updated the code 👍
Co-Authored-By: Addison Stavlo <Stavz01@gmail.com>
Co-Authored-By: Addison Stavlo <Stavz01@gmail.com>
dc14757
to
1f78a8b
Compare
1f78a8b
to
b483b24
Compare
Description
This PR adds the Undo redo UI to the edit-site and edit-widgets screens, making them consistent with edit-post.
The undo-redo does not work well on these screens yet. Some problems are:
This PR just adds the UI, so the issues we have become noticeable. I will follow up with some bug fixes to the problems we currently have. I think exposing the UI even if undo-redo is not working well is a first step to fix all the issues.
How has this been tested?
I verified the undo and redo buttons appeared on the edit-site and edit-widgets screens.