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

Editor: Implement meta as custom source #16402

Merged
merged 34 commits into from
Jul 10, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
34 commits
Select commit Hold shift + click to select a range
e33523c
Editor: Implement meta as custom source
aduth Jul 2, 2019
81bdcb1
Block Editor: Skip outgoing sync next value only if matches by reference
aduth Jul 5, 2019
6f58b3e
Editor: Implement editor tearDown to un-ready state
aduth Jul 5, 2019
df30e18
Editor: Add subscription action generator for managing sources depend…
aduth Jul 5, 2019
6bfb9b0
Editor: Track source dependencies by registry
aduth Jul 8, 2019
2a6ef11
Editor: Prepare initial blocks reset for applied post-sourced values
aduth Jul 8, 2019
9b99c78
Editor: Yield editor setup action after state preparation
aduth Jul 8, 2019
db51f77
Editor: Update block sources as part of reset step
aduth Jul 8, 2019
e1786a1
Editor: Retrieve registry at most once in applying sourced attributes
aduth Jul 8, 2019
c2e0c1f
Editor: Account for source values apply / update in inner blocks
aduth Jul 8, 2019
ba1a8cd
Editor: Update block sources documentation per interface changes
aduth Jul 8, 2019
f4f7403
Block Editor: Add lastBlockAttributesChange unit tests
aduth Jul 8, 2019
19618fd
Block Editor: Document, test getLastBlockAttributesChange selector
aduth Jul 8, 2019
b5b0990
Editor: Fix block sources getDependencies grammar
aduth Jul 9, 2019
9100f39
Editor: Fix block sources meta getDependencies JSDoc typo
aduth Jul 9, 2019
de45deb
Editor: Align block sources getDependencies JSDoc tags
aduth Jul 9, 2019
d318a82
Editor: Resolve awaitNextStateChange JSDoc typo
aduth Jul 9, 2019
eab1ee5
Editor: Resolve getRegistry JSDoc typo
aduth Jul 9, 2019
6472638
Block Editor: Always unset expected outbound sync value in provider u…
aduth Jul 9, 2019
7263e11
Editor: Detect block attributes change as direct result of last action
aduth Jul 9, 2019
d643af0
Documentation: Regenerate editor, block-editor data documentation
aduth Jul 9, 2019
d9c95e0
Block Editor: Unset outbound sync value in editor provider reset cond…
aduth Jul 9, 2019
54c1f48
Editor: Update getDependencies JSDoc to separate yield grouping
aduth Jul 9, 2019
2247600
Editor: Dispatch SETUP_EDITOR prior to blocks reset
aduth Jul 9, 2019
814e04a
Editor: Update actions tests per reordered SETUP_EDITOR
aduth Jul 9, 2019
c237ccb
Block API: Stub default attributes, keywords values for block type re…
aduth Jul 9, 2019
3e097ef
Block API: Update documentation per formation of WPBlockTypeIconDescr…
aduth Jul 9, 2019
c12bee4
Editor: Iterate last block changes in updating sources
aduth Jul 9, 2019
ac2acaf
Editor: Update source even if already updated for block updates
aduth Jul 9, 2019
075ffab
Editor: Iterate updated attributes from block reset source update
aduth Jul 10, 2019
6f66257
Editor: Mark custom sources selectors, actions as experimental
aduth Jul 10, 2019
8516fd1
Editor: Remove redundant Object#hasOwnProperty in iterating attributes
aduth Jul 10, 2019
37a7ba9
Block Editor: Rename getLastBlockAttributesChange to getLastBlockAttr…
aduth Jul 10, 2019
468f908
Editor: Use getRegistry control for next change subscription
aduth Jul 10, 2019
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
40 changes: 3 additions & 37 deletions packages/block-editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { get, reduce, size, first, last } from 'lodash';
import { first, last } from 'lodash';
import { animated } from 'react-spring/web.cjs';

/**
Expand Down Expand Up @@ -658,42 +658,8 @@ const applyWithDispatch = withDispatch( ( dispatch, ownProps, { select } ) => {

return {
setAttributes( newAttributes ) {
const { name, clientId } = ownProps;
const type = getBlockType( name );

function isMetaAttribute( key ) {
return get( type, [ 'attributes', key, 'source' ] ) === 'meta';
}

// Partition new attributes to delegate update behavior by source.
//
// TODO: A consolidated approach to external attributes sourcing
// should be devised to avoid specific handling for meta, enable
// additional attributes sources.
//
// See: https://github.com/WordPress/gutenberg/issues/2759
const {
blockAttributes,
metaAttributes,
} = reduce( newAttributes, ( result, value, key ) => {
if ( isMetaAttribute( key ) ) {
result.metaAttributes[ type.attributes[ key ].meta ] = value;
} else {
result.blockAttributes[ key ] = value;
}

return result;
}, { blockAttributes: {}, metaAttributes: {} } );

if ( size( blockAttributes ) ) {
updateBlockAttributes( clientId, blockAttributes );
}

if ( size( metaAttributes ) ) {
const { getSettings } = select( 'core/block-editor' );
const onChangeMeta = getSettings().__experimentalMetaSource.onChange;
onChangeMeta( metaAttributes );
}
const { clientId } = ownProps;
updateBlockAttributes( clientId, newAttributes );
},
onSelect( clientId = ownProps.clientId, initialPosition ) {
selectBlock( clientId, initialPosition );
Expand Down
40 changes: 40 additions & 0 deletions packages/block-editor/src/components/provider/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
BlockEditorProvider
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ I love this README :)

===================

BlockEditorProvider is a component which establishes a new block editing context, and serves as the entry point for a new block editor. It is implemented as a [controlled input](https://reactjs.org/docs/forms.html#controlled-components), expected to receive a value of a blocks array, calling `onChange` and/or `onInput` when the user interacts to change blocks in the editor. It is intended to be used as a wrapper component, where its children comprise the user interface through which a user modifies the blocks value, notably via other components made available from this `block-editor` module.

## Props

### `value`

* **Type:** `Array`
* **Required** `no`
Copy link
Contributor

Choose a reason for hiding this comment

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

The component will try to use all of these props (except for "children") and throw errors. We should mark them as required.

Copy link
Member Author

Choose a reason for hiding this comment

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

The component will try to use all of these props (except for "children") and throw errors. We should mark them as required.

I was actually thinking we should make them optional. At least in the case of onInput and onChange, it's quite likely most usage will only care to provide one or the other.

I agree that the documentation here is not accurate per the current implementation. I didn't want to document an undesirable requirement, however. Maybe we should seek to make it optional as a separate task to this pull request.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense 👍


The current array of blocks.

### `onChange`

* **Type:** `Function`
* **Required** `no`

A callback invoked when the blocks have been modified in a persistent manner. Contrasted with `onInput`, a "persistent" change is one which is not an extension of a composed input. Any update to a distinct block or block attribute is treated as persistent.
Copy link
Contributor

Choose a reason for hiding this comment

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

...change is one which is not an extension of a composed input...

This is a bit hard to grasp. Maybe an example would help.


The distinction between these two callbacks is akin to the [differences between `input` and `change` events](https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/input_event) in the DOM API:

>The input event is fired every time the value of the element changes. **This is unlike the change event, which only fires when the value is committed**, such as by pressing the enter key, selecting a value from a list of options, and the like.

In the context of an editor, an example usage of this distinction is for managing a history of blocks values (an "Undo"/"Redo" mechanism). While value updates should always be reflected immediately (`onInput`), you may only want history entries to reflect change milestones (`onChange`).

### `onInput`

* **Type:** `Function`
* **Required** `no`

A callback invoked when the blocks have been modified in a non-persistent manner. Contrasted with `onChange`, a "non-persistent" change is one which is part of a composed input. Any sequence of updates to the same block attribute are treated as non-persistent, except for the first.
Copy link
Contributor

Choose a reason for hiding this comment

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

Any sequence of updates to the same block attribute are treated as non-persistent, except for the first.

Why is this desired? If I edit a color twice, the second edit wouldn't persist?

Copy link
Member Author

@aduth aduth Jul 3, 2019

Choose a reason for hiding this comment

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

Why is this desired? If I edit a color twice, the second edit wouldn't persist?

I think the choice of "persistent" as the term here can be potentially misleading. The intent was for it to be akin to the distinction between input and change events in the DOM API:

The input event is fired every time the value of the element changes. This is unlike the change event, which only fires when the value is committed, such as by pressing the enter key, selecting a value from a list of options, and the like.

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/input_event
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event

In the context of the editor, the use-case is for Undo levels. You don't want Cmd+Z to undo paragraph changes one character at a time, but rather as units (in our case, reflected as sequences of updates to the same block attribute).

When I was first thinking about this documentation, I had in mind to explicitly mention how it's used for Undo/Redo in the editor, but neglected to write it when I'd returned to my computer 😄

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that's much clearer now. I remember running into that code, but forgot about it when reading this.


### `children`

* **Type:** `WPElement`
* **Required** `no`

Children elements for which the BlockEditorProvider context should apply.
23 changes: 18 additions & 5 deletions packages/block-editor/src/components/provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,21 @@ class BlockEditorProvider extends Component {
this.attachChangeObserver( registry );
}

if ( this.isSyncingOutcomingValue ) {
this.isSyncingOutcomingValue = false;
if ( this.isSyncingOutcomingValue !== null && this.isSyncingOutcomingValue === value ) {
// Skip block reset if the value matches expected outbound sync
// triggered by this component by a preceding change detection.
// Only skip if the value matches expectation, since a reset should
// still occur if the value is modified (not equal by reference),
// to allow that the consumer may apply modifications to reflect
// back on the editor.
this.isSyncingOutcomingValue = null;
} else if ( value !== prevProps.value ) {
this.isSyncingIncomingValue = true;
// Reset changing value in all other cases than the sync described
// above. Since this can be reached in an update following an out-
// bound sync, unset the outbound value to avoid considering it in
// subsequent renders.
this.isSyncingOutcomingValue = null;
this.isSyncingIncomingValue = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know these changes are really tricky and fragile. The good news is that we have e2e tests for most of the use-cases and you can endup fixing one but breaking others :P

resetBlocks( value );
}
}
Expand Down Expand Up @@ -79,15 +90,17 @@ class BlockEditorProvider extends Component {
onChange,
onInput,
} = this.props;

const newBlocks = getBlocks();
const newIsPersistent = isLastBlockChangePersistent();

if (
newBlocks !== blocks && (
this.isSyncingIncomingValue ||
__unstableIsLastBlockChangeIgnored()
)
) {
this.isSyncingIncomingValue = false;
this.isSyncingIncomingValue = null;
blocks = newBlocks;
isPersistent = newIsPersistent;
return;
Expand All @@ -101,7 +114,7 @@ class BlockEditorProvider extends Component {
// When knowing the blocks value is changing, assign instance
// value to skip reset in subsequent `componentDidUpdate`.
if ( newBlocks !== blocks ) {
this.isSyncingOutcomingValue = true;
this.isSyncingOutcomingValue = newBlocks;
}

blocks = newBlocks;
Expand Down
78 changes: 41 additions & 37 deletions packages/block-editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,29 +195,6 @@ export function isUpdatingSameBlockAttribute( action, lastAction ) {
);
}

/**
* Higher-order reducer intended to reset the cache key of all blocks
* whenever the post meta values change.
*
* @param {Function} reducer Original reducer function.
*
* @return {Function} Enhanced reducer function.
*/
const withPostMetaUpdateCacheReset = ( reducer ) => ( state, action ) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️

const newState = reducer( state, action );
const previousMetaValues = get( state, [ 'settings', '__experimentalMetaSource', 'value' ] );
const nextMetaValues = get( newState.settings.__experimentalMetaSource, [ 'value' ] );
// If post meta values change, reset the cache key for all blocks
if ( previousMetaValues !== nextMetaValues ) {
newState.blocks = {
...newState.blocks,
cache: mapValues( newState.blocks.cache, () => ( {} ) ),
};
}

return newState;
};

/**
* Utility returning an object with an empty object value for each key.
*
Expand Down Expand Up @@ -1228,17 +1205,44 @@ export const blockListSettings = ( state = {}, action ) => {
return state;
};

export default withPostMetaUpdateCacheReset(
combineReducers( {
blocks,
isTyping,
isCaretWithinFormattedText,
blockSelection,
blocksMode,
blockListSettings,
insertionPoint,
template,
settings,
preferences,
} )
);
/**
* Reducer return an updated state representing the most recent block attribute
* update. The state is structured as an object where the keys represent the
* client IDs of blocks, the values a subset of attributes from the most recent
* block update. The state is always reset to null if the last action is
* anything other than an attributes update.
*
* @param {Object<string,Object>} state Current state.
* @param {Object} action Action object.
*
* @return {[string,Object]} Updated state.
*/
export function lastBlockAttributesChange( state, action ) {
switch ( action.type ) {
case 'UPDATE_BLOCK':
if ( ! action.updates.attributes ) {
break;
}

return { [ action.clientId ]: action.updates.attributes };

case 'UPDATE_BLOCK_ATTRIBUTES':
return { [ action.clientId ]: action.attributes };
}
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens for other actions that change multiple blocks, like RESET_BLOCKS... (Thinking about a third-party plugin triggering that action and not the block editor provider), should we try to list the changes or do we just trigger a full sync? What happens now?

Copy link
Member Author

Choose a reason for hiding this comment

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

What happens for other actions that change multiple blocks, like RESET_BLOCKS... (Thinking about a third-party plugin triggering that action and not the block editor provider), should we try to list the changes or do we just trigger a full sync? What happens now?

It wouldn't try to sync meta attributes from the updates, which is the same as how it behaves today in master. I think it might be a bit difficult to infer from the reset whether those meta values should be considered canonical, or whether the source should be applied again by the editor. The latter is what I expect happens in the current state of this branch.


return null;
}

export default combineReducers( {
blocks,
isTyping,
isCaretWithinFormattedText,
blockSelection,
blocksMode,
blockListSettings,
insertionPoint,
template,
settings,
preferences,
lastBlockAttributesChange,
} );
71 changes: 15 additions & 56 deletions packages/block-editor/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,14 +64,6 @@ const MILLISECONDS_PER_WEEK = 7 * 24 * 3600 * 1000;
*/
const EMPTY_ARRAY = [];

/**
* Shared reference to an empty object for cases where it is important to avoid
* returning a new object reference on every invocation.
*
* @type {Object}
*/
const EMPTY_OBJECT = {};

/**
* Returns a block's name given its client ID, or null if no block exists with
* the client ID.
Expand Down Expand Up @@ -108,42 +100,14 @@ export function isBlockValid( state, clientId ) {
*
* @return {Object?} Block attributes.
*/
export const getBlockAttributes = createSelector(
( state, clientId ) => {
const block = state.blocks.byClientId[ clientId ];
if ( ! block ) {
return null;
}

let attributes = state.blocks.attributes[ clientId ];

// Inject custom source attribute values.
//
// TODO: Create generic external sourcing pattern, not explicitly
// targeting meta attributes.
const type = getBlockType( block.name );
if ( type ) {
attributes = reduce( type.attributes, ( result, value, key ) => {
if ( value.source === 'meta' ) {
if ( result === attributes ) {
result = { ...result };
}

result[ key ] = getPostMeta( state, value.meta );
}

return result;
}, attributes );
}
export function getBlockAttributes( state, clientId ) {
const block = state.blocks.byClientId[ clientId ];
if ( ! block ) {
return null;
}

return attributes;
},
( state, clientId ) => [
state.blocks.byClientId[ clientId ],
state.blocks.attributes[ clientId ],
getPostMeta( state ),
]
);
return state.blocks.attributes[ clientId ];
}

/**
* Returns a block given its client ID. This is a parsed copy of the block,
Expand Down Expand Up @@ -193,7 +157,7 @@ export const __unstableGetBlockWithoutInnerBlocks = createSelector(
},
( state, clientId ) => [
state.blocks.byClientId[ clientId ],
...getBlockAttributes.getDependants( state, clientId ),
state.blocks.attributes[ clientId ],
]
);

Expand Down Expand Up @@ -296,7 +260,6 @@ export const getBlocksByClientId = createSelector(
( clientId ) => getBlock( state, clientId )
),
( state ) => [
getPostMeta( state ),
state.blocks.byClientId,
state.blocks.order,
state.blocks.attributes,
Expand Down Expand Up @@ -656,7 +619,6 @@ export const getMultiSelectedBlocks = createSelector(
state.blocks.byClientId,
state.blocks.order,
state.blocks.attributes,
getPostMeta( state ),
]
);

Expand Down Expand Up @@ -1421,19 +1383,16 @@ export function __unstableIsLastBlockChangeIgnored( state ) {
}

/**
* Returns the value of a post meta from the editor settings.
* Returns the block attributes changed as a result of the last dispatched
* action.
*
* @param {Object} state Global application state.
* @param {string} key Meta Key to retrieve
* @param {Object} state Block editor state.
*
* @return {*} Meta value
* @return {Object<string,Object>} Subsets of block attributes changed, keyed
* by block client ID.
*/
function getPostMeta( state, key ) {
if ( key === undefined ) {
return get( state, [ 'settings', '__experimentalMetaSource', 'value' ], EMPTY_OBJECT );
}

return get( state, [ 'settings', '__experimentalMetaSource', 'value', key ] );
export function __experimentalGetLastBlockAttributeChanges( state ) {
return state.lastBlockAttributesChange;
}

/**
Expand Down
Loading