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

Block Editor: Fix undo level inconsistencies. #17259

Merged
merged 7 commits into from
Sep 16, 2019
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/core-data/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
"@wordpress/api-fetch": "file:../api-fetch",
"@wordpress/data": "file:../data",
"@wordpress/deprecated": "file:../deprecated",
"@wordpress/is-shallow-equal": "file:../is-shallow-equal",
"@wordpress/url": "file:../url",
"equivalent-key-map": "^0.2.2",
"lodash": "^4.17.14",
Expand Down
39 changes: 17 additions & 22 deletions packages/core-data/src/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import { keyBy, map, groupBy, flowRight, isEqual, get } from 'lodash';
* WordPress dependencies
*/
import { combineReducers } from '@wordpress/data';
import isShallowEqual from '@wordpress/is-shallow-equal';

/**
* Internal dependencies
Expand Down Expand Up @@ -322,32 +323,26 @@ export function undo( state = UNDO_INITIAL_STATE, action ) {
return nextState;
}

let nextState;
if ( state.length === 0 ) {
// Create an initial entry so that we can undo to it.
nextState = [
{
kind: action.meta.undo.kind,
name: action.meta.undo.name,
recordId: action.meta.undo.recordId,
edits: { ...state.flattenedUndo, ...action.meta.undo.edits },
},
];
nextState.offset = 0;
return nextState;
}

// Clear potential redos, because this only supports linear history.
nextState = state.slice( 0, state.offset || undefined );
const nextState = state.slice( 0, state.offset || undefined );
nextState.offset = 0;

nextState.pop();
nextState.push( {
kind: action.kind,
name: action.name,
recordId: action.recordId,
edits: { ...action.edits, ...state.flattenedUndo },
kind: action.meta.undo.kind,
name: action.meta.undo.name,
recordId: action.meta.undo.recordId,
edits: { ...state.flattenedUndo, ...action.meta.undo.edits },
} );

const comparisonUndoEdits = Object.values( action.meta.undo.edits ).filter( ( edit ) => typeof edit !== 'function' );
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is this checking for? Would be great to inline comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That the last two edits are not the same.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm more wondering what the filter is for. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When an edit is a function it's an optimization to avoid running some expensive operation. We can't rely on the function references being the same so we opt out of comparing them here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That could use an inline comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm working on it with another fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

const comparisonEdits = Object.values( action.edits ).filter( ( edit ) => typeof edit !== 'function' );
if ( ! isShallowEqual( comparisonUndoEdits, comparisonEdits ) ) {
nextState.push( {
kind: action.kind,
name: action.name,
recordId: action.recordId,
edits: action.edits,
} );
}
return nextState;
}

Expand Down
17 changes: 17 additions & 0 deletions packages/e2e-tests/specs/undo.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,10 @@ describe( 'undo', () => {
await pressKeyWithModifier( 'primary', 'z' );

expect( await getEditedPostContent() ).toMatchSnapshot();

await pressKeyWithModifier( 'primary', 'z' );

expect( await getEditedPostContent() ).toBe( '' );
} );

it( 'should undo typing after non input change', async () => {
Expand All @@ -43,6 +47,10 @@ describe( 'undo', () => {
await pressKeyWithModifier( 'primary', 'z' );

expect( await getEditedPostContent() ).toMatchSnapshot();

await pressKeyWithModifier( 'primary', 'z' );

expect( await getEditedPostContent() ).toBe( '' );
} );

it( 'Should undo to expected level intervals', async () => {
Expand Down Expand Up @@ -100,4 +108,13 @@ describe( 'undo', () => {
const visibleContent = await page.evaluate( () => document.activeElement.textContent );
expect( visibleContent ).toBe( 'original' );
} );

it( 'should not create undo level when saving', async () => {
await clickBlockAppender();
await page.keyboard.type( '1' );
await saveDraft();
await pressKeyWithModifier( 'primary', 'z' );

expect( await getEditedPostContent() ).toBe( '' );
} );
} );
8 changes: 7 additions & 1 deletion packages/editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,13 @@ export function* setupEditor( post, edits, template ) {
};
yield resetEditorBlocks( blocks, { __unstableShouldCreateUndoLevel: false } );
yield setupEditorState( post );
if ( edits ) {
if (
edits &&
Object.keys( edits ).some(
( key ) =>
edits[ key ] !== ( has( post, [ key, 'raw' ] ) ? post[ key ].raw : post[ key ] )
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The raw key seems very API specific. Given all the API abstractions, is this the right place to check for the raw key?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's not ideal. I think we should deprecate initial edits if possible.

)
) {
yield editPost( edits );
}
yield* __experimentalSubscribeSources();
Expand Down