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.

Sign up for GitHub

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

Recursively step through edits to track individually changed post meta #10827

Merged
merged 6 commits into from
Nov 19, 2018
Merged

Recursively step through edits to track individually changed post meta #10827

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
3d0cce5
Only send edited meta values as state to editPost
earnjam Oct 19, 2018
1d8a708
Recursively step through edits to granularly update state on EDIT_POST
earnjam Oct 19, 2018
59953d3
Don't recurse for arrays
earnjam Oct 19, 2018
d76a250
Editor: Deeply diff/merge edited property edits/updates
aduth Oct 23, 2018
7b6bccd
Editor: Pass meta updates from block as patch
aduth Oct 23, 2018
40a96ee
Editor: Shallow merge on whitelisted post property edits
aduth Nov 8, 2018
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
7 changes: 1 addition & 6 deletions packages/editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,10 +171,7 @@ export class BlockListBlock extends Component {
}, {} );

if ( size( metaAttributes ) ) {
this.props.onMetaChange( {
...this.props.meta,
...metaAttributes,
} );
this.props.onMetaChange( metaAttributes );
}
}

Expand Down Expand Up @@ -593,7 +590,6 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV
isTyping,
isCaretWithinFormattedText,
getBlockIndex,
getEditedPostAttribute,
getBlockMode,
isSelectionEnabled,
getSelectedBlocksInitialCaretPosition,
Expand All @@ -619,7 +615,6 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV
isTypingWithinBlock: ( isSelected || isParentOfSelectedBlock ) && isTyping(),
isCaretWithinFormattedText: isCaretWithinFormattedText(),
order: getBlockIndex( clientId, rootClientId ),
meta: getEditedPostAttribute( 'meta' ),
mode: getBlockMode( clientId ),
isSelectionEnabled: isSelectionEnabled(),
initialPosition: getSelectedBlocksInitialCaretPosition(),
Expand Down
21 changes: 19 additions & 2 deletions packages/editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,16 @@ import {
} from './defaults';
import { insertAt, moveTo } from './array';

/**
* Set of post properties for which edits should assume a merging behavior,
* assuming an object value.
*
* @type {Set}
*/
const EDIT_MERGE_PROPERTIES = new Set( [
'meta',
] );

/**
* Returns a post attribute value, flattening nested rendered content using its
* raw value in place of its original object form.
Expand Down Expand Up @@ -244,7 +254,14 @@ export const editor = flow( [
// Only assign into result if not already same value
if ( value !== state[ key ] ) {
result = getMutateSafeObject( state, result );
result[ key ] = value;

if ( EDIT_MERGE_PROPERTIES.has( key ) ) {
// Merge properties should assign to current value.
result[ key ] = { ...result[ key ], ...value };
} else {
// Otherwise override.
result[ key ] = value;
}
}

return result;
Expand All @@ -264,7 +281,7 @@ export const editor = flow( [
( key ) => getPostRawValue( action.post[ key ] );

return reduce( state, ( result, value, key ) => {
if ( value !== getCanonicalValue( key ) ) {
if ( ! isEqual( value, getCanonicalValue( key ) ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Granting there's one odd edge case for which this doesn't cover: If updates are applied, individual keys won't be unset unless all meta values are now equal with their new canonical values. e.g.

{ type: 'EDIT_POST', edits: { meta: { a: 1, b: 2 } } }
// { edits: { meta: { a: 1, b: 2 } } }

{ type: 'UPDATE_POST', edits: { meta: { a: 1 } } }
// { edits: { meta: { a: 1, b: 2 } } }

{ type: 'UPDATE_POST', edits: { meta: { a: 1, b: 2 } } }
// { edits: {} }

In practice, I don't think this would ever occur (UPDATE_POST is called on save with all edited values assumed as being the new canonical), and the impact being that it would continue to save edited values which had already been saved before.

The alternative would be an implementation which would individually unset keys and remove the top-level key once empty.

return result;
}

Expand Down
84 changes: 84 additions & 0 deletions packages/editor/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1040,6 +1040,90 @@ describe( 'state', () => {
} );
} );

it( 'should merge object values', () => {
const original = editor( undefined, {
type: 'EDIT_POST',
edits: {
meta: {
a: 1,
},
},
} );

const state = editor( original, {
type: 'EDIT_POST',
edits: {
meta: {
b: 2,
},
},
} );

expect( state.present.edits ).toEqual( {
meta: {
a: 1,
b: 2,
},
} );
} );

it( 'return state by reference on unchanging update', () => {
const original = editor( undefined, {} );

const state = editor( original, {
type: 'UPDATE_POST',
edits: {},
} );

expect( state.present.edits ).toBe( original.present.edits );
} );

it( 'unset reset post values which match by canonical value', () => {
const original = editor( undefined, {
type: 'EDIT_POST',
edits: {
title: 'modified title',
},
} );

const state = editor( original, {
type: 'RESET_POST',
post: {
title: {
raw: 'modified title',
},
},
} );

expect( state.present.edits ).toEqual( {} );
} );

it( 'unset reset post values by deep match', () => {
const original = editor( undefined, {
type: 'EDIT_POST',
edits: {
title: 'modified title',
meta: {
a: 1,
b: 2,
},
},
} );

const state = editor( original, {
type: 'UPDATE_POST',
edits: {
title: 'modified title',
meta: {
a: 1,
b: 2,
},
},
} );

expect( state.present.edits ).toEqual( {} );
} );

it( 'should omit content when resetting', () => {
// Use case: When editing in Text mode, we defer to content on
// the property, but we reset blocks by parse when switching
Expand Down