Skip to content

Commit

Permalink
Blocks: Focus the right position when merging blocks
Browse files Browse the repository at this point in the history
Focus the end of the previous block before merging content
  • Loading branch information
youknowriad committed May 1, 2017
1 parent 37ea746 commit 43f3efb
Show file tree
Hide file tree
Showing 2 changed files with 15 additions and 11 deletions.
16 changes: 9 additions & 7 deletions blocks/components/editable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -153,12 +153,8 @@ export default class Editable extends wp.element.Component {
if ( this.props.onMerge && event.keyCode === KEYCODE_BACKSPACE && this.isStartOfEditor() ) {
this.onChange();
this.props.onMerge( this.editor.getContent() );

// If merge causes editor to be removed, stop other callbacks from
// trying to handle the event
if ( this.editor.removed ) {
event.stopImmediatePropagation();
}
event.preventDefault();
event.stopImmediatePropagation();

This comment has been minimized.

Copy link
@aduth

aduth May 1, 2017

Member

I'm curious about this one. What if a block doesn't handle onMerge? Does it make sense in that case to prevent default handlers and stop propagation?

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 1, 2017

Author Contributor

We're inside the if ( this.props.onMerge ) so we're certain at this moment that the block handles the onMerge

This comment has been minimized.

Copy link
@aduth

aduth May 1, 2017

Member

We're inside the if ( this.props.onMerge ) so we're certain at this moment that the block handles the onMerge

Oh, good call. I guess the more specific point is that we don't necessarily know what the block is doing for its onMerge (could be noop for all we know). Stopping the event handlers seems to assume that something will be destroyed.

This comment has been minimized.

Copy link
@youknowriad

youknowriad May 1, 2017

Author Contributor

Right! I think you won't like the alternative I have here. Adding a setTimeout before calling onMerge :). The issue here is that a START_TYPING is triggered right after the onMerge unless I stop the event.

}
}

Expand Down Expand Up @@ -266,8 +262,14 @@ export default class Editable extends wp.element.Component {
}

focus() {
if ( this.props.focus ) {
const { focus } = this.props;
if ( focus ) {
this.editor.focus();
// Offset = -1 means we should focus the end of the editable
if ( focus.offset === -1 ) {

This comment has been minimized.

Copy link
@aduth

aduth May 1, 2017

Member

Clever solution. Won't help in the case where we need to focus somewhere within the content, but it's not obvious yet whether we'll need that use case.

this.editor.selection.select( this.editor.getBody(), true );
this.editor.selection.collapse( false );
}
}
}

Expand Down
10 changes: 6 additions & 4 deletions editor/modes/visual-editor/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import { connect } from 'react-redux';
import classnames from 'classnames';
import { Slot } from 'react-slot-fill';
import { partial } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -63,7 +64,7 @@ class VisualEditorBlock extends wp.element.Component {
}

mergeWithPrevious() {
const { block, previousBlock, onRemove, onChange } = this.props;
const { block, previousBlock, onRemove, onChange, onFocus } = this.props;

// Do nothing when it's the first block
if ( ! previousBlock ) {
Expand All @@ -90,6 +91,7 @@ class VisualEditorBlock extends wp.element.Component {

// Calling the merge to update the attributes and remove the block to be merged
const updatedAttributes = previousBlockSettings.merge( previousBlock.attributes, blockWithTheSameType.attributes );
onFocus( previousBlock.uid, { offset: -1 } );
onChange( previousBlock.uid, {
attributes: {
...previousBlock.attributes,
Expand Down Expand Up @@ -174,7 +176,7 @@ class VisualEditorBlock extends wp.element.Component {
attributes={ block.attributes }
setAttributes={ this.setAttributes }
insertBlockAfter={ onInsertAfter }
setFocus={ onFocus }
setFocus={ partial( onFocus, block.uid ) }

This comment has been minimized.

Copy link
@aduth

aduth May 1, 2017

Member

I'd caution against using partial based on intent to deprecate in the next major version. Arrow function can serve as a substitute:

setFocus={ () => onFocus( block.uid ) }
mergeWithPrevious={ this.mergeWithPrevious }
/>
</div>
Expand Down Expand Up @@ -247,10 +249,10 @@ export default connect(
} );
},

onFocus( config ) {
onFocus( uid, config ) {
dispatch( {
type: 'UPDATE_FOCUS',
uid: ownProps.uid,
uid,
config
} );
},
Expand Down

0 comments on commit 43f3efb

Please sign in to comment.