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

Try a generic block editor module #13088

Merged
merged 22 commits into from
Feb 22, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
150e0ed
Try a generic block editor module
youknowriad Dec 24, 2018
44ca3aa
Migrate persisted block usage
youknowriad Jan 28, 2019
f6a0e17
Block Editor: Remove history handling as managed by Editor
aduth Feb 13, 2019
3e0fc58
Block Editor: Separate onChange from onInput as committing change
aduth Feb 18, 2019
25772de
Move state.editor.blocks to state.blocks
youknowriad Feb 19, 2019
2d06c4f
Avoid rerendering the editor if the settings didn't change
youknowriad Feb 19, 2019
ba7b92d
Switch the position of the isSyncingBlockValue flag
youknowriad Feb 19, 2019
30d4f81
Block Editor: Reimplement persistent block change as reducer state
aduth Feb 19, 2019
d904406
Block Editor: Avoid handling non-updating actions in isUpdatingSameBl…
aduth Feb 20, 2019
f1dff88
fixup: Block Editor: Reimplement persistent block change as reducer s…
aduth Feb 20, 2019
97bdb82
Undo level and block change in the same dispatch
youknowriad Feb 21, 2019
3a0ee11
More stable preformatted e2e test
youknowriad Feb 21, 2019
5aa7de7
Try a more stable container blocks test
youknowriad Feb 21, 2019
01a154c
Fix RichText undo interactions
youknowriad Feb 21, 2019
d463228
More stable preformatted block test
youknowriad Feb 21, 2019
14e2dfa
Avoid unnecessary getBlocks selector invalidation
youknowriad Feb 21, 2019
3ebc2c6
Try fixing initial dirtiness
youknowriad Feb 21, 2019
724b38f
Block Editor: Align hasSameKeys to editor implementation
aduth Feb 21, 2019
bd355c5
Editor: Rename resetEditorBlocks undo argument as unstable
aduth Feb 21, 2019
9aecc91
Block Editor: Rename markLastChangeAsPersistent as unstable
aduth Feb 21, 2019
184807b
Block Editor: Assure lastAction is non-undefined
aduth Feb 21, 2019
123f7b2
Editor: Assign dependencies upon block-editor
aduth Feb 21, 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
Prev Previous commit
Next Next commit
Fix RichText undo interactions
  • Loading branch information
youknowriad committed Feb 21, 2019
commit 01a154cecd4dd657559fe1175b0a35d2fb6fb0ac
Original file line number Diff line number Diff line change
Expand Up @@ -1043,4 +1043,8 @@ in order to switch its temporary id with the real id.
*Parameters*

* id: Reusable block's id.
* updatedId: Updated block's id.
* updatedId: Updated block's id.

### markLastChangeAsPersistent

Returns an action object used in signalling that the last block change should be marked explicitely as persistent.
125 changes: 97 additions & 28 deletions packages/block-editor/src/components/provider/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,14 +3,37 @@
*/
import { Component } from '@wordpress/element';
import { DropZoneProvider, SlotFillProvider } from '@wordpress/components';
import { withDispatch, withSelect } from '@wordpress/data';
import { compose } from '@wordpress/compose';
import { withDispatch, RegistryConsumer } from '@wordpress/data';
import { createHigherOrderComponent, compose } from '@wordpress/compose';

/**
* Higher-order component which renders the original component with the current
* registry context passed as its `registry` prop.
*
* @param {WPComponent} OriginalComponent Original component.
*
* @return {WPComponent} Enhanced component.
*/
const withRegistry = createHigherOrderComponent(
Copy link
Member

Choose a reason for hiding this comment

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

Aside: It seems a common enough pattern that we should consider one or both of opting for useContext hook from React 16.8.0, and/or our own implementation of a withContext higher-order component which would behave similarly.

The main difference with a higher-order component is that we'd need to instruct the prop name to which the context value should be assigned. One or both of:

withContext( 'registry', RegistryContext )( MyComponent );
withContext( { registry: RegistryContext } )( MyComponent );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should try hooks for a moment and see whether adding higher order components still make sense or not.

( OriginalComponent ) => ( props ) => (
<RegistryConsumer>
{ ( registry ) => (
<OriginalComponent
{ ...props }
registry={ registry }
/>
) }
</RegistryConsumer>
),
'withRegistry'
);

class BlockEditorProvider extends Component {
componentDidMount() {
this.isSyncingBlockValue = true;
this.props.updateEditorSettings( this.props.settings );
this.isSyncingIncomingValue = true;
this.props.resetBlocks( this.props.value );
this.attachChangeObserver( this.props.registry );
}

componentDidUpdate( prevProps ) {
Expand All @@ -19,32 +42,88 @@ class BlockEditorProvider extends Component {
updateEditorSettings,
value,
resetBlocks,
blocks,
onInput,
onChange,
isLastBlockChangePersistent,
registry,
} = this.props;

if ( settings !== prevProps.settings ) {
updateEditorSettings( settings );
}

if ( this.isSyncingBlockValue ) {
this.isSyncingBlockValue = false;
} else if ( blocks !== prevProps.blocks ) {
this.isSyncingBlockValue = true;
if ( registry !== prevProps.registry ) {
this.attachChangeObserver( registry );
}

if ( isLastBlockChangePersistent ) {
onChange( blocks );
} else {
onInput( blocks );
}
if ( this.isSyncingOutcomingValue ) {
this.isSyncingOutcomingValue = false;
} else if ( value !== prevProps.value ) {
this.isSyncingBlockValue = true;
this.isSyncingIncomingValue = true;
resetBlocks( value );
}
}

componentWillUnmount() {
if ( this.unsubscribe ) {
this.unsubscribe();
}
}

/**
* Given a registry object, overrides the default dispatch behavior for the
* `core/block-editor` store to interpret a state change and decide whether
* we should call `onChange` or `onInput` depending on whether the change
* is persistent or not.
*
* This needs to be done synchronously after state changes (instead of using
* `componentDidUpdate`) in order to avoid batching these changes.
*
* @param {WPDataRegistry} registry Registry from which block editor
* dispatch is to be overriden.
*/
attachChangeObserver( registry ) {
if ( this.unsubscribe ) {
this.unsubscribe();
}

const {
getBlocks,
isLastBlockChangePersistent,
} = registry.select( 'core/block-editor' );

let blocks = getBlocks();
let isPersistent = isLastBlockChangePersistent();

this.unsubscribe = registry.subscribe( () => {
const {
onChange,
onInput,
} = this.props;
const newBlocks = getBlocks();
const newIsPersistent = isLastBlockChangePersistent();
if ( newBlocks !== blocks && this.isSyncingIncomingValue ) {
this.isSyncingIncomingValue = false;
blocks = newBlocks;
isPersistent = newIsPersistent;
return;
}

if (
newBlocks !== blocks ||
// This happens when a previous input is explicitely marked as persistent.
( newIsPersistent && ! isPersistent )
) {
blocks = newBlocks;
isPersistent = newIsPersistent;

this.isSyncingOutcomingValue = true;
Copy link
Member

Choose a reason for hiding this comment

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

Something I was starting to not like about my previous implementation was having a hard assumption that the parent component was rendering BlockEditor as controlled, i.e. assuming that onInput would cause the next rendered value to be the one we pass as blocks here. It seems like something which should regularly be true, but the lack of guarantee makes it feel a bit fragile.

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 agree with you and this is what was causing the e2e tests without the last commit but it was specific to the initial call.

If we want it to be less fragile, we'd have to call getBlocks right after the resetBlocks call (I was doing this initially in the PR)

if ( isPersistent ) {
Copy link
Member

@aduth aduth Feb 21, 2019

Choose a reason for hiding this comment

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

This being an if else between onChange and onInput was something I was curious about as well, whether a persistent change should fire onChange instead of or in addition to an onInput.

Depending how much we care to align to the default DOM input behaviors (important for setting expectations), a changed input fires both change and input. Try toggling the radio buttons here, for example:

https://codepen.io/aduth/pen/exboNb

Edit: Or I guess, a better way to put it, in the DOM, a change event will always be preceded by an input event.

Copy link
Contributor Author

@youknowriad youknowriad Feb 21, 2019

Choose a reason for hiding this comment

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

This was essentially my way of making the value change and the history level fire on the same dispatch. We can probably still do it even if have both events at the same time, but it requires more tweaks from the caller.

onChange( blocks );
} else {
onInput( blocks );
}
}
} );
}

render() {
const { children } = this.props;

Expand All @@ -59,17 +138,6 @@ class BlockEditorProvider extends Component {
}

export default compose( [
withSelect( ( select ) => {
const {
getBlocks,
isLastBlockChangePersistent,
} = select( 'core/block-editor' );

return {
blocks: getBlocks(),
isLastBlockChangePersistent: isLastBlockChangePersistent(),
};
} ),
withDispatch( ( dispatch ) => {
const {
updateEditorSettings,
Expand All @@ -81,4 +149,5 @@ export default compose( [
resetBlocks,
};
} ),
withRegistry,
] )( BlockEditorProvider );
10 changes: 10 additions & 0 deletions packages/block-editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -532,3 +532,13 @@ export function __unstableSaveReusableBlock( id, updatedId ) {
updatedId,
};
}

/**
* Returns an action object used in signalling that the last block change should be marked explicitely as persistent.
*
* @return {Object} Action object.
*/
export function markLastChangeAsPersistent() {
return { type: 'MARK_LAST_CHANGE_AS_PERSISTENT' };
}

16 changes: 11 additions & 5 deletions packages/block-editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -186,11 +186,17 @@ function withPersistentBlockChange( reducer ) {
let lastAction;

return ( state, action ) => {
const nextState = reducer( state, action );
if ( state !== nextState ) {
nextState.isPersistentChange = (
! isUpdatingSameBlockAttribute( action, lastAction )
);
let nextState = reducer( state, action );
const isExplicitPersistentChange = action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT';

if ( state !== nextState || isExplicitPersistentChange ) {
nextState = {
...nextState,
isPersistentChange: (
isExplicitPersistentChange ||
! isUpdatingSameBlockAttribute( action, lastAction )
),
};
}

lastAction = action;
Expand Down
10 changes: 3 additions & 7 deletions packages/editor/src/components/rich-text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1102,17 +1102,13 @@ const RichTextContainer = compose( [
} ),
withDispatch( ( dispatch ) => {
const {
createUndoLevel,
redo,
undo,
markLastChangeAsPersistent,
enterFormattedText,
exitFormattedText,
} = dispatch( 'core/editor' );
} = dispatch( 'core/block-editor' );

return {
onCreateUndoLevel: createUndoLevel,
onRedo: redo,
onUndo: undo,
onCreateUndoLevel: markLastChangeAsPersistent,
onEnterFormattedText: enterFormattedText,
onExitFormattedText: exitFormattedText,
};
Expand Down