Skip to content
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

Merged

Conversation

jorgefilipecosta
Copy link
Member

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:

  • On the widget screen, when applying changes to multiple areas, sometimes the undo just undo the changes in one of the areas (I think it is a bug in core-data).
  • On the widget scree on paragraphs, it seems the undo is character by character (I think we are probably mishandling changes in BlockEditorProvider inside widgets screen).
  • On the edit-site screen, it appears the undo works in some cases but ignores changes in some template areas.

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.

@jorgefilipecosta jorgefilipecosta added [Type] Enhancement A suggestion for improvement. [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Feature] History History, undo, redo, revisions, autosave. labels Apr 28, 2020
@github-actions
Copy link

github-actions bot commented Apr 28, 2020

Size Change: +1.13 kB (0%)

Total Size: 820 kB

Filename Size Change
build/annotations/index.js 3.62 kB +2 B (0%)
build/api-fetch/index.js 4.08 kB +1 B
build/block-directory/index.js 6.6 kB +2 B (0%)
build/block-editor/index.js 101 kB +2 B (0%)
build/block-library/index.js 115 kB -2 B (0%)
build/blocks/index.js 48.1 kB +1 B
build/data-controls/index.js 1.29 kB +2 B (0%)
build/data/index.js 8.44 kB +6 B (0%)
build/date/index.js 5.47 kB -3 B (0%)
build/edit-post/index.js 28.1 kB +2 B (0%)
build/edit-site/index.js 11.9 kB +560 B (4%)
build/edit-widgets/index.js 8.33 kB +559 B (6%) 🔍
build/editor/index.js 44.3 kB -3 B (0%)
build/escape-html/index.js 734 B +1 B
build/format-library/index.js 7.63 kB -4 B (0%)
build/hooks/index.js 2.13 kB -1 B
build/is-shallow-equal/index.js 710 B -2 B (0%)
build/keyboard-shortcuts/index.js 2.51 kB +2 B (0%)
build/keycodes/index.js 1.94 kB +1 B
build/list-reusable-blocks/index.js 3.12 kB -1 B
build/primitives/index.js 1.5 kB +2 B (0%)
build/redux-routine/index.js 2.85 kB +3 B (0%)
build/server-side-render/index.js 2.67 kB -1 B
build/url/index.js 4.02 kB -2 B (0%)
ℹ️ View Unchanged
Filename Size Change
build/a11y/index.js 1.02 kB 0 B
build/autop/index.js 2.82 kB 0 B
build/blob/index.js 620 B 0 B
build/block-directory/style-rtl.css 760 B 0 B
build/block-directory/style.css 761 B 0 B
build/block-editor/style-rtl.css 10.2 kB 0 B
build/block-editor/style.css 10.2 kB 0 B
build/block-library/editor-rtl.css 7.08 kB 0 B
build/block-library/editor.css 7.08 kB 0 B
build/block-library/style-rtl.css 7.22 kB 0 B
build/block-library/style.css 7.23 kB 0 B
build/block-library/theme-rtl.css 683 B 0 B
build/block-library/theme.css 685 B 0 B
build/block-serialization-default-parser/index.js 1.88 kB 0 B
build/block-serialization-spec-parser/index.js 3.1 kB 0 B
build/components/index.js 179 kB 0 B
build/components/style-rtl.css 16.9 kB 0 B
build/components/style.css 16.9 kB 0 B
build/compose/index.js 6.66 kB 0 B
build/core-data/index.js 11.4 kB 0 B
build/deprecated/index.js 772 B 0 B
build/dom-ready/index.js 568 B 0 B
build/dom/index.js 3.1 kB 0 B
build/edit-navigation/index.js 4.05 kB 0 B
build/edit-navigation/style-rtl.css 485 B 0 B
build/edit-navigation/style.css 485 B 0 B
build/edit-post/style-rtl.css 12.2 kB 0 B
build/edit-post/style.css 12.2 kB 0 B
build/edit-site/style-rtl.css 5.18 kB 0 B
build/edit-site/style.css 5.18 kB 0 B
build/edit-widgets/style-rtl.css 4.67 kB 0 B
build/edit-widgets/style.css 4.66 kB 0 B
build/editor/editor-styles-rtl.css 428 B 0 B
build/editor/editor-styles.css 431 B 0 B
build/editor/style-rtl.css 5.07 kB 0 B
build/editor/style.css 5.08 kB 0 B
build/element/index.js 4.65 kB 0 B
build/format-library/style-rtl.css 502 B 0 B
build/format-library/style.css 502 B 0 B
build/html-entities/index.js 622 B 0 B
build/i18n/index.js 3.56 kB 0 B
build/list-reusable-blocks/style-rtl.css 226 B 0 B
build/list-reusable-blocks/style.css 226 B 0 B
build/media-utils/index.js 5.29 kB 0 B
build/notices/index.js 1.79 kB 0 B
build/nux/index.js 3.4 kB 0 B
build/nux/style-rtl.css 616 B 0 B
build/nux/style.css 613 B 0 B
build/plugins/index.js 2.56 kB 0 B
build/priority-queue/index.js 789 B 0 B
build/rich-text/index.js 14.8 kB 0 B
build/shortcode/index.js 1.7 kB 0 B
build/token-list/index.js 1.28 kB 0 B
build/viewport/index.js 1.84 kB 0 B
build/warning/index.js 1.14 kB 0 B
build/wordcount/index.js 1.18 kB 0 B

compressed-size-action

@vindl vindl requested review from noahtallen and ockham April 29, 2020 01:00
@youknowriad youknowriad mentioned this pull request Apr 29, 2020
53 tasks
@mtias mtias mentioned this pull request Apr 29, 2020
17 tasks
import { undo as undoIcon } from '@wordpress/icons';
import { displayShortcut } from '@wordpress/keycodes';

export default function UndoButton() {
Copy link
Member

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?

Copy link
Member

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?

Copy link
Contributor

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.

@noahtallen
Copy link
Member

appears the undo works in some cases but ignores changes in some template areas.

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 😁

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a 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.

@jorgefilipecosta jorgefilipecosta force-pushed the add/undo-redo-UI-to-edit-site-and-edit-widgets branch 2 times, most recently from b56aae1 to 87823dd Compare April 30, 2020 11:20
@jorgefilipecosta
Copy link
Member Author

Hi @noahtallen, @Addison-Stavlo thank you for the comments, review, and suggestions the changes were applied 👍

@vindl
Copy link
Member

vindl commented Apr 30, 2020

Currently the tests are failing with There are local uncommitted changes after one or both of 'npm install' or 'npm run docs:build'!. It's probably needs npm install and committed changes to package-lock.json.

@noahtallen
Copy link
Member

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:

  • I click cmd-z a ton of times in a row
  • undo does not happen
  • I open the "Edit" menu in the macOS toolbar and then do cmd-z again (or click the Undo menu item there)
  • undo happens

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

@Addison-Stavlo
Copy link
Contributor

Addison-Stavlo commented Apr 30, 2020

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,

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:

import '@wordpress/keyboard-shortcuts';

<KeyboardShortcuts
shortcuts={ preventEventDiscovery }
/>

So may be related to that?

Copy link
Member

@noahtallen noahtallen left a 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 :)

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo left a 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',
Copy link
Contributor

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?

Copy link
Member Author

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 👍

@jorgefilipecosta jorgefilipecosta force-pushed the add/undo-redo-UI-to-edit-site-and-edit-widgets branch from dc14757 to 1f78a8b Compare May 4, 2020 15:33
@jorgefilipecosta jorgefilipecosta force-pushed the add/undo-redo-UI-to-edit-site-and-edit-widgets branch from 1f78a8b to b483b24 Compare May 4, 2020 16:43
@jorgefilipecosta jorgefilipecosta merged commit b092bd7 into master May 4, 2020
@jorgefilipecosta jorgefilipecosta deleted the add/undo-redo-UI-to-edit-site-and-edit-widgets branch May 4, 2020 17:31
@github-actions github-actions bot added this to the Gutenberg 8.1 milestone May 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] History History, undo, redo, revisions, autosave. [Feature] Widgets Screen The block-based screen that replaced widgets.php. [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants