Skip to content

/ gutenberg Public

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

Jump to bottom

Bogus undo and unsaved changes state #34802

Open
Mamaduka opened this issue Sep 14, 2021 · 6 comments · May be fixed by #34839
Open

Bogus undo and unsaved changes state #34802

Mamaduka opened this issue Sep 14, 2021 · 6 comments · May be fixed by #34839
Assignees
@talldan
Labels
[Feature] Navigation Screen [Status] In Progress [Type] Bug
Projects
Navigation editor

Comments

@Mamaduka
Copy link
Member

@Mamaduka Mamaduka commented Sep 14, 2021

Problem

When you open the Navigation Editor or reload the page after saving (CMD + R), the page is displayed if you have unsaved changes. It also shows the active "Undo" button, which does nothing.

Screencast

CleanShot.2021-09-14.at.13.11.37.mp4
@Mamaduka Mamaduka added this to Inbox in Navigation editor via automation Sep 14, 2021
@Mamaduka
Copy link
Member Author

@Mamaduka Mamaduka commented Sep 14, 2021

I think this "side effect" causes the issue. If I comment out the setAttribute, there's no more unsaved state.

gutenberg/packages/block-library/src/navigation-link/edit.js

Lines 366 to 373 in 639a603

useEffect( () => {
// This side-effect should not create an undo level as those should
// only be created via user interactions. Mark this change as
// not persistent to avoid undo level creation.
// See https://github.com/WordPress/gutenberg/issues/34564.
__unstableMarkNextChangeAsNotPersistent();
setAttributes( { isTopLevelLink } );
}, [ isTopLevelLink ] );

Is it possible for __unstableMarkNextChangeAsNotPersistent not to work in context of Navigation Screen?

/cc @talldan.

@talldan
Copy link
Contributor

@talldan talldan commented Sep 15, 2021 •
edited

You're right, it doesn't seem to be working in the navigation editor. I'm not sure why that would be though. Looking into it.

An option might just be to get rid of isTopLevelLink. I'm not sure why it should be needed, I think it should be possible to determine using a CSS selector whether something is a top level link. Using an attribute for this isn't great.

@Mamaduka
Copy link
Member Author

@Mamaduka Mamaduka commented Sep 15, 2021

I'm not sure about isTopLevelLink. Based on #31149, we need that attribute to have different color options for submenus.

Side note: If we end up keeping isTopLevelLink, we should wrap setAttributes in if condition and only set it for actual top links.

@talldan
Copy link
Contributor

@talldan talldan commented Sep 15, 2021 •
edited

So there are a couple of differences between the post editor and the nav editor in terms of this issue:

1. Block is loaded differently

In the post editor isTopLevelLink is saved as an attribute so when loading a post this value is restored. setAttributes( { isTopLevelLink } ) has no effect because the attribute already has the correct value. Contrast this to the navigation editor, blocks are converted to menu items when saving, isTopLevelLink isn't saved anywhere. When loading the block always has an undefined value, so now setAttributes( { isTopLevelLink } ) actually does trigger a change. It could be possible to set a correct value for isTopLevelLink in our menuItemToBlock code, perhaps true when the parent is 0, or false otherwise.

2. Nav editor behaves slightly differently to the post editor in adding things to the undo stack.

The reducer that adds things to the undo stack works slightly differently in the post editor. There, changes to blocks are considered 'transient' edits, and the edit is flattened onto the previous item:

gutenberg/packages/core-data/src/reducer.js

Lines 448 to 464 in 79be738

// Transient edits don't create an undo level, but are
// reachable in the next meaningful edit to which they
// are merged. They are defined in the entity's config.
if (
! isCreateUndoLevel &&
! Object.keys( action.edits ).some(
( key ) => ! action.transientEdits[ key ]
)
) {
nextState = [ ...state ];
nextState.flattenedUndo = {
...state.flattenedUndo,
...action.edits,
};
nextState.offset = state.offset;
return nextState;
}

This is defined in the entities definition, like this one for widgets:

gutenberg/packages/core-data/src/entities.js

Lines 71 to 79 in 79be738

{
name: 'widget',
kind: 'root',
baseURL: '/wp/v2/widgets',
baseURLParams: { context: 'edit' },
plural: 'widgets',
transientEdits: { blocks: true },
label: __( 'Widgets' ),
},

I don't think the nav editor has a definition for its 'stub' navigation post entity, so maybe it needs one and to define block edits as transient.

@talldan talldan linked a pull request that will close this issue Sep 15, 2021
Draft
7 tasks
@Mamaduka
Copy link
Member Author

@Mamaduka Mamaduka commented Sep 15, 2021

Thanks for the detailed explanation.

The Widget editor also uses an in-memory post entity but does have the same issue.

@talldan
Copy link
Contributor

@talldan talldan commented Sep 15, 2021 •
edited

Just testing the widget editor, it does indeed have the same issue. Preventing the nav block from serialising isTopLevelLink (by deleting the attribute declaration) makes it become apparent.

I've put together a fix in #34839, but it introduces another issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@talldan talldan

Labels
[Feature] Navigation Screen [Status] In Progress [Type] Bug
Projects
Navigation editor
  
Inbox
Milestone
No milestone
Linked pull requests

Successfully merging a pull request may close this issue.

Fix navigation editor transient edits
2 participants
@Mamaduka @talldan