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

Blocks: Handle pressing backspace at the beginning of blocks #461

Merged
merged 9 commits into from
Apr 27, 2017
35 changes: 34 additions & 1 deletion blocks/components/editable/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import './style.scss';
// as we're doing here; instead, we should consider a common components path.
import Toolbar from '../../../editor/components/toolbar';

const KEYCODE_BACKSPACE = 8;
const htmlToReactParser = new HtmlToReactParser();
const formatMap = {
strong: 'bold',
Expand Down Expand Up @@ -51,7 +52,7 @@ export default class Editable extends wp.element.Component {
this.bindEditorNode = this.bindEditorNode.bind( this );
this.onFocus = this.onFocus.bind( this );
this.onNodeChange = this.onNodeChange.bind( this );

this.onKeyDown = this.onKeyDown.bind( this );
this.state = {
formats: {}
};
Expand Down Expand Up @@ -85,6 +86,7 @@ export default class Editable extends wp.element.Component {
editor.on( 'NewBlock', this.onNewBlock );
editor.on( 'focusin', this.onFocus );
editor.on( 'nodechange', this.onNodeChange );
editor.on( 'keydown', this.onKeyDown );
}

onInit() {
Expand All @@ -110,6 +112,37 @@ export default class Editable extends wp.element.Component {
this.props.onChange( this.getContent() );
}

isStartOfEditor() {
const range = this.editor.selection.getRng();
if ( range.startOffset !== 0 || ! range.collapsed ) {
return false;
}
const start = range.startContainer;
const body = this.editor.getBody();
let element = start;
while ( element !== body ) {
const child = element;
element = element.parentNode;
if ( element.firstChild !== child ) {
return false;
}
}
return true;
}

onKeyDown( event ) {
if ( this.props.onMerge && event.keyCode === KEYCODE_BACKSPACE && this.isStartOfEditor() ) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't know if it was intentional, but nice optimization having the heaviest part of the condition performed last and taking advantage of short-circuiting 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Intentional :)

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();
}
}
}

onNewBlock() {
if ( this.props.tagName || ! this.props.onSplit ) {
return;
Expand Down
61 changes: 34 additions & 27 deletions blocks/library/heading/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -31,32 +31,6 @@ registerBlock( 'core/heading', {
} ) )
],

edit( { attributes, setAttributes, focus, setFocus } ) {
const { content, nodeName = 'H2', align } = attributes;

return (
<Editable
tagName={ nodeName.toLowerCase() }
value={ content }
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { content: value } ) }
style={ align ? { textAlign: align } : null }
/>
);
},

save( { attributes } ) {
const { align, nodeName = 'H2', content } = attributes;
const Tag = nodeName.toLowerCase();

return (
<Tag style={ align ? { textAlign: align } : null }>
{ content }
</Tag>
);
},

transforms: {
from: [
{
Expand Down Expand Up @@ -88,5 +62,38 @@ registerBlock( 'core/heading', {
}
}
]
}
},

merge( attributes, attributesToMerge ) {
return {
content: wp.element.concatChildren( attributes.content, attributesToMerge.content )
};
},

edit( { attributes, setAttributes, focus, setFocus, mergeWithPrevious } ) {
const { content, nodeName = 'H2', align } = attributes;

return (
<Editable
tagName={ nodeName.toLowerCase() }
value={ content }
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { content: value } ) }
style={ align ? { textAlign: align } : null }
onMerge={ mergeWithPrevious }
/>
);
},

save( { attributes } ) {
const { align, nodeName = 'H2', content } = attributes;
const Tag = nodeName.toLowerCase();

return (
<Tag style={ align ? { textAlign: align } : null }>
{ content }
</Tag>
);
},
} );
9 changes: 8 additions & 1 deletion blocks/library/text/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,13 @@ registerBlock( 'core/text', {
}
],

edit( { attributes, setAttributes, insertBlockAfter, focus, setFocus } ) {
merge( attributes, attributesToMerge ) {
return {
content: wp.element.concatChildren( attributes.content, attributesToMerge.content )
};
},

edit( { attributes, setAttributes, insertBlockAfter, focus, setFocus, mergeWithPrevious } ) {
const { content = <p />, align } = attributes;

return (
Expand All @@ -64,6 +70,7 @@ registerBlock( 'core/text', {
content: after
} ) );
} }
onMerge={ mergeWithPrevious }
/>
);
},
Expand Down
73 changes: 62 additions & 11 deletions editor/modes/visual-editor/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ class VisualEditorBlock extends wp.element.Component {
this.setAttributes = this.setAttributes.bind( this );
this.maybeDeselect = this.maybeDeselect.bind( this );
this.maybeHover = this.maybeHover.bind( this );
this.mergeWithPrevious = this.mergeWithPrevious.bind( this );
this.previousOffset = null;
}

Expand All @@ -38,7 +39,7 @@ class VisualEditorBlock extends wp.element.Component {

setAttributes( attributes ) {
const { block, onChange } = this.props;
onChange( {
onChange( block.uid, {
attributes: {
...block.attributes,
...attributes
Expand All @@ -61,6 +62,44 @@ class VisualEditorBlock extends wp.element.Component {
}
}

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

// Do nothing when it's the first block
if ( ! previousBlock ) {
return;
}

const previousBlockSettings = wp.blocks.getBlockSettings( previousBlock.blockType );

// Remove the previous block if it's not mergeable
if ( ! previousBlockSettings.merge ) {
onRemove( previousBlock.uid );
return;
}

// We can only merge blocks with similar types
// thus, we transform the block to merge first
const blockWithTheSameType = previousBlock.blockType === block.blockType
? block
: wp.blocks.switchToBlockType( block, previousBlock.blockType );

// If the block types can not match, do nothing
if ( ! blockWithTheSameType ) {
return;
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to remove the previous block here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. I don't really know the right answer. I think this use-case might not happen as often as we think because I'm assuming the majority of textual blocks could be transformed in between'em but It's a possibility.

I preferred to avoid deletion and do nothing but I don't have strong arguments for one or another.

Copy link
Member

Choose a reason for hiding this comment

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

My main reason for bringing it up was merely as a point of consistency, since it seemed from the above condition that the "unhandleable" case should simply be removed.

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 not considering the "merge not defined" as "unhandleable" but more like, if you want this block to be removed when we hit backspace, don't define the merge

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 not considering the "merge not defined" as "unhandleable" but more like, if you want this block to be removed when we hit backspace, don't define the merge

Isn't that the condition above though, not this one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I misread your previous message as "if you don't want this block to be removed". Disregard.

Copy link
Member

Choose a reason for hiding this comment

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

This is fine, but we should probably document the behavior somewhere.

Copy link
Member

Choose a reason for hiding this comment

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

I'd very much prefer that we don't silently delete content here. Submitted a PR to change this behavior - #530

}

// Calling the merge to update the attributes and remove the block to be merged
const updatedAttributes = previousBlockSettings.merge( previousBlock.attributes, blockWithTheSameType.attributes );
onChange( previousBlock.uid, {
attributes: {
...previousBlock.attributes,
...updatedAttributes
}
} );
onRemove( block.uid );
}

componentDidUpdate() {
if ( this.previousOffset ) {
window.scrollTo(
Expand Down Expand Up @@ -137,6 +176,7 @@ class VisualEditorBlock extends wp.element.Component {
setAttributes={ this.setAttributes }
insertBlockAfter={ onInsertAfter }
setFocus={ onFocus }
mergeWithPrevious={ this.mergeWithPrevious }
/>
</div>
);
Expand All @@ -145,19 +185,23 @@ class VisualEditorBlock extends wp.element.Component {
}

export default connect(
( state, ownProps ) => ( {
order: state.blocks.order.indexOf( ownProps.uid ),
block: state.blocks.byUid[ ownProps.uid ],
isSelected: state.selectedBlock.uid === ownProps.uid,
isHovered: state.hoveredBlock === ownProps.uid,
focus: state.selectedBlock.uid === ownProps.uid ? state.selectedBlock.focus : null,
isTyping: state.selectedBlock.uid === ownProps.uid ? state.selectedBlock.typing : false,
} ),
( state, ownProps ) => {
const order = state.blocks.order.indexOf( ownProps.uid );
return {
previousBlock: state.blocks.byUid[ state.blocks.order[ order - 1 ] ] || null,
block: state.blocks.byUid[ ownProps.uid ],
isSelected: state.selectedBlock.uid === ownProps.uid,
isHovered: state.hoveredBlock === ownProps.uid,
focus: state.selectedBlock.uid === ownProps.uid ? state.selectedBlock.focus : null,
isTyping: state.selectedBlock.uid === ownProps.uid ? state.selectedBlock.typing : false,
order
};
},
( dispatch, ownProps ) => ( {
onChange( updates ) {
onChange( uid, updates ) {
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I don't love that we have some props that accept a uid and others that assume it from the other props. I understand the need for it and don't have a great alternative.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think making the uid mandatory even for callbacks we're using only for the current block (and using _.partial ) is a good move?

Copy link
Member

Choose a reason for hiding this comment

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

I like that idea because it promotes consistency, but I don't like that idea because it loses the sense that the component is meant to be specific to a single block.

dispatch( {
type: 'UPDATE_BLOCK',
uid: ownProps.uid,
uid,
updates
} );
},
Expand Down Expand Up @@ -210,6 +254,13 @@ export default connect(
uid: ownProps.uid,
config
} );
},

onRemove( uid ) {
dispatch( {
type: 'REMOVE_BLOCK',
uid
} );
}
} )
)( VisualEditorBlock );
8 changes: 7 additions & 1 deletion editor/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import { combineReducers, createStore } from 'redux';
import { keyBy, last } from 'lodash';
import { keyBy, last, omit, without } from 'lodash';

/**
* Internal dependencies
Expand Down Expand Up @@ -43,6 +43,9 @@ export const blocks = combineUndoableReducers( {
...state,
[ action.uid ]: action.block
};

case 'REMOVE_BLOCK':
return omit( state, action.uid );
}

return state;
Expand Down Expand Up @@ -87,6 +90,9 @@ export const blocks = combineUndoableReducers( {
action.uid,
...state.slice( index + 2 )
];

case 'REMOVE_BLOCK':
return without( state, action.uid );
}

return state;
Expand Down
28 changes: 28 additions & 0 deletions editor/test/state.js
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,34 @@ describe( 'state', () => {

expect( state.order ).to.equal( original.order );
} );

it( 'should remove the block', () => {
const original = blocks( undefined, {
type: 'REPLACE_BLOCKS',
blockNodes: [ {
uid: 'chicken',
blockType: 'core/test-block',
attributes: {}
}, {
uid: 'ribs',
blockType: 'core/test-block',
attributes: {}
} ]
} );
const state = blocks( original, {
type: 'REMOVE_BLOCK',
uid: 'chicken'
} );

expect( state.order ).to.eql( [ 'ribs' ] );
expect( state.byUid ).to.eql( {
ribs: {
uid: 'ribs',
blockType: 'core/test-block',
attributes: {}
}
} );
} );
} );

describe( 'hoveredBlock()', () => {
Expand Down
22 changes: 22 additions & 0 deletions element/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,25 @@ export function renderToString( element ) {

return renderToStaticMarkup( element );
}

/**
* Concatenate two or more React children objects
*
* @param {...?Object} childrens Set of children to concatenate
* @return {Array} The concatenated value
*/
export function concatChildren( ...childrens ) {
return childrens.reduce( ( memo, children, i ) => {
Children.forEach( children, ( child, j ) => {
if ( 'string' !== typeof child ) {
child = cloneElement( child, {
key: [ i, j ].join()
} );
}

memo.push( child );
} );

return memo;
}, [] );
}
Loading