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

Jump to bottom

Test: Move the merge block handler to the actions file and unit test it #769

Merged
merged 2 commits into from
May 16, 2017
Merged

Test: Move the merge block handler to the actions file and unit test it #769

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0bd4d0d
Test: Move the merge block handler to the actions file and unit test it
youknowriad May 11, 2017
f12e179
Blocks: Handle forward backspace
youknowriad May 12, 2017
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
29 changes: 27 additions & 2 deletions blocks/editable/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import FormatToolbar from './format-toolbar';
import TinyMCE from './tinymce';

const KEYCODE_BACKSPACE = 8;
const KEYCODE_DELETE = 46;

const alignmentMap = {
alignleft: 'left',
Expand Down Expand Up @@ -177,10 +178,34 @@ export default class Editable extends wp.element.Component {
return true;
}

isEndOfEditor() {
const range = this.editor.selection.getRng();
if ( range.endOffset !== range.endContainer.textContent.length || ! range.collapsed ) {
return false;
}
const start = range.endContainer;
const body = this.editor.getBody();
let element = start;
while ( element !== body ) {
const child = element;
element = element.parentNode;
if ( element.lastChild !== child ) {
return false;
}
}
return true;
}

onKeyDown( event ) {
if ( this.props.onMerge && event.keyCode === KEYCODE_BACKSPACE && this.isStartOfEditor() ) {
if (
this.props.onMerge && (
( event.keyCode === KEYCODE_BACKSPACE && this.isStartOfEditor() ) ||
( event.keyCode === KEYCODE_DELETE && this.isEndOfEditor() )
)
) {
const forward = event.keyCode === KEYCODE_DELETE;
this.onChange();
this.props.onMerge( this.editor.getContent() );
this.props.onMerge( forward );
Copy link
Member

@aduth aduth May 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I really don't like the idea of Editable assuming a context with the editor's store, but it sure would be nice if onMerge here were the connected dispatch, wouldn't it? 😄 Since it means blocks wouldn't need to pass this through at all.

Copy link
Contributor Author

@youknowriad youknowriad May 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, but it's not that simple. Imagine a block having multiple editables. So the merge of the last one would probably merge its content with the previous one (or do nothing) but the merge of the first editable would actually trigger the merge with the previous block.

It could be way simpler if it's one block = one editable, but that's not the case. Could we provide more "metadata" on how the block uses the editable in this case and fallback to one editable mergeable with the previous block if this metadata is not provided? I don't know but that's something we should think about.

example:

edit() {
  return (
    <div>
       <Editable value={ attributes.content } mergeWithPrevious />
       <Editable onMerge={ mergeWithAttribute( 'content' ) } />
       <Editable noMerge />
    </div>
  );
}

Copy link
Member

Choose a reason for hiding this comment

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

Imagine a block having multiple editables.

Ah, that's a good point. I hadn't considered this.

Could we provide more "metadata" on how the block uses the editable

Interesting idea. I think at the point where we're saying a block implementer needs to be concerned with merge behavior (and it's fairly obvious they do), I'm not as much concerned about accepting and applying the onMerge prop handling anymore.

It could be slightly more optimized if Editable were able to handle this by hints, but I'm not sure a clean way we can connect the Editable to the editor's store without tying it directly to the editor (not so great).

Copy link
Member

Choose a reason for hiding this comment

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

To your points here, do you think it's obvious to the block implementer what's happening when they pass mergeBlocks to an editable, specifically in merging with a previous block, next block, or between editables of the same block?

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 think this should be well documented. It's not obvious at all for now but I don't have any alternative. We should enhance our block API documentation once it's more stable.

Copy link
Member

Choose a reason for hiding this comment

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

cc @mtias since we'd chatted about this topic today. Maybe for the sake of simplicity we don't allow complex merging within a block, and try to make editor smarter about knowing how to handle Editables. It's a very fuzzy desire, but maybe traversing returned element to find first or last editable to decide how to merge into previous or next. @mtias indicated some desire to avoid merging and transforms being responsibility of block at all, which sounds nice; difficult to pull off 😄 But I think it'd be fair to make some compromises about what merging can be performed if it helps simplify.

I don't think this needs to be settled here; in fact, mergeBlocks could very well be compatible with this future desire. Just a matter of using context to communicate directly between VisualEditorBlock and Editable to avoid block needing to handle directly.

Copy link
Member

Choose a reason for hiding this comment

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

I missed this comment @aduth. Let's create an issue for this so we don't lose sight of it.

Copy link
Member

Choose a reason for hiding this comment

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

I missed this comment @aduth. Let's create an issue for this so we don't lose sight of it.

See #878

event.preventDefault();
event.stopImmediatePropagation();
}
Expand Down
4 changes: 2 additions & 2 deletions blocks/library/heading/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ registerBlock( 'core/heading', {
};
},

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

return (
Expand All @@ -89,7 +89,7 @@ registerBlock( 'core/heading', {
focus={ focus }
onFocus={ setFocus }
onChange={ ( value ) => setAttributes( { content: value } ) }
onMerge={ mergeWithPrevious }
onMerge={ mergeBlocks }
inline
/>
);
Expand Down
4 changes: 2 additions & 2 deletions blocks/library/quote/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ registerBlock( 'core/quote', {
],
},

edit( { attributes, setAttributes, focus, setFocus, mergeWithPrevious } ) {
edit( { attributes, setAttributes, focus, setFocus, mergeBlocks } ) {
const { value, citation, style = 1 } = attributes;
const focusedEditable = focus ? focus.editable || 'value' : null;

Expand All @@ -97,7 +97,7 @@ registerBlock( 'core/quote', {
}
focus={ focusedEditable === 'value' ? focus : null }
onFocus={ () => setFocus( { editable: 'value' } ) }
onMerge={ mergeWithPrevious }
onMerge={ mergeBlocks }
showAlignments
/>
{ ( ( citation && citation.length > 0 ) || !! focus ) && (
Expand Down
4 changes: 2 additions & 2 deletions blocks/library/text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ registerBlock( 'core/text', {
};
},

edit( { attributes, setAttributes, insertBlockAfter, focus, setFocus, mergeWithPrevious } ) {
edit( { attributes, setAttributes, insertBlockAfter, focus, setFocus, mergeBlocks } ) {
const { content } = attributes;

return (
Expand All @@ -46,7 +46,7 @@ registerBlock( 'core/text', {
content: after,
} ) );
} }
onMerge={ mergeWithPrevious }
onMerge={ mergeBlocks }
showAlignments
/>
);
Expand Down
55 changes: 55 additions & 0 deletions editor/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,22 @@
import { get } from 'lodash';
import { parse, stringify } from 'querystring';

export function focusBlock( uid, config ) {
return {
type: 'UPDATE_FOCUS',
uid,
config,
};
}

export function replaceBlocks( uids, blocks ) {
return {
type: 'REPLACE_BLOCKS',
uids,
blocks,
};
}

export function savePost( dispatch, postId, edits ) {
const toSend = postId ? { id: postId, ...edits } : edits;
const isNew = ! postId;
Expand Down Expand Up @@ -41,3 +57,42 @@ export function savePost( dispatch, postId, edits ) {
} );
} );
}

export function mergeBlocks( dispatch, blockA, blockB ) {
Copy link
Member

Choose a reason for hiding this comment

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

Similar to what we did with concatChildren, I wonder if this needs to be necessarily limited to merging two blocks (unless it becomes much more complex).

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 not as easy as concatChildren, because merging blocks could produce several blocks, merging only the first paragraph. I'm thinking we could do this recursively, but I don't see much value to support this for now.

const blockASettings = wp.blocks.getBlockSettings( blockA.blockType );

// Only focus the previous block if it's not mergeable
if ( ! blockASettings.merge ) {
dispatch( focusBlock( blockA.uid ) );
return;
}

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

// If the block types can not match, do nothing
if ( ! blocksWithTheSameType || ! blocksWithTheSameType.length ) {
return;
}

// Calling the merge to update the attributes and remove the block to be merged
const updatedAttributes = blockASettings.merge( blockA.attributes, blocksWithTheSameType[ 0 ].attributes );

dispatch( focusBlock( blockA.uid, { offset: -1 } ) );
dispatch( replaceBlocks(
[ blockA.uid, blockB.uid ],
[
{
...blockA,
attributes: {
...blockA.attributes,
...updatedAttributes,
},
},
...blocksWithTheSameType.slice( 1 ),
]
) );
}
10 changes: 5 additions & 5 deletions editor/block-switcher/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import Dashicon from 'components/dashicon';
* Internal dependencies
*/
import './style.scss';
import { replaceBlocks } from '../actions';
import { getBlock } from '../selectors';

class BlockSwitcher extends wp.element.Component {
Expand Down Expand Up @@ -111,11 +112,10 @@ export default connect(
} ),
( dispatch, ownProps ) => ( {
onTransform( block, blockType ) {
dispatch( {
type: 'REPLACE_BLOCKS',
uids: [ ownProps.uid ],
blocks: wp.blocks.switchToBlockType( block, blockType ),
} );
dispatch( replaceBlocks(
[ ownProps.uid ],
wp.blocks.switchToBlockType( block, blockType )
) );
},
} )
)( clickOutside( BlockSwitcher ) );
71 changes: 19 additions & 52 deletions editor/modes/visual-editor/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,10 @@ import Toolbar from 'components/toolbar';
*/
import BlockMover from '../../block-mover';
import BlockSwitcher from '../../block-switcher';
import { focusBlock, mergeBlocks } from '../../actions';
import {
getPreviousBlock,
getNextBlock,
getBlock,
getBlockFocus,
getBlockOrder,
Expand All @@ -35,7 +37,7 @@ class VisualEditorBlock extends wp.element.Component {
this.maybeHover = this.maybeHover.bind( this );
this.maybeStartTyping = this.maybeStartTyping.bind( this );
this.removeOnBackspace = this.removeOnBackspace.bind( this );
this.mergeWithPrevious = this.mergeWithPrevious.bind( this );
this.mergeBlocks = this.mergeBlocks.bind( this );
this.previousOffset = null;
}

Expand Down Expand Up @@ -100,50 +102,22 @@ class VisualEditorBlock extends wp.element.Component {
}
}

mergeWithPrevious() {
const { block, previousBlock, onFocus, replaceBlocks } = this.props;
mergeBlocks( forward = false ) {
const { block, previousBlock, nextBlock, onMerge } = this.props;

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

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

// Do nothing if the previous block is not mergeable
if ( ! previousBlockSettings.merge ) {
onFocus( previousBlock.uid );
if (
( ! forward && ! previousBlock ) ||
( forward && ! nextBlock )
) {
return;
}

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

// If the block types can not match, do nothing
if ( ! blocksWithTheSameType || ! blocksWithTheSameType.length ) {
return;
if ( forward ) {
onMerge( block, nextBlock );
} else {
onMerge( previousBlock, block );
}

// Calling the merge to update the attributes and remove the block to be merged
const updatedAttributes = previousBlockSettings.merge( previousBlock.attributes, blocksWithTheSameType[ 0 ].attributes );

onFocus( previousBlock.uid, { offset: -1 } );
replaceBlocks(
[ previousBlock.uid, block.uid ],
[
{
...previousBlock,
attributes: {
...previousBlock.attributes,
...updatedAttributes,
},
},
...blocksWithTheSameType.slice( 1 ),
]
);
}

componentDidUpdate( prevProps ) {
Expand Down Expand Up @@ -234,7 +208,7 @@ class VisualEditorBlock extends wp.element.Component {
setAttributes={ this.setAttributes }
insertBlockAfter={ onInsertAfter }
setFocus={ partial( onFocus, block.uid ) }
mergeWithPrevious={ this.mergeWithPrevious }
mergeBlocks={ this.mergeBlocks }
/>
</div>
</div>
Expand All @@ -247,6 +221,7 @@ export default connect(
( state, ownProps ) => {
return {
previousBlock: getPreviousBlock( state, ownProps.uid ),
nextBlock: getNextBlock( state, ownProps.uid ),
block: getBlock( state, ownProps.uid ),
isSelected: isBlockSelected( state, ownProps.uid ),
isHovered: isBlockHovered( state, ownProps.uid ),
Expand Down Expand Up @@ -306,12 +281,8 @@ export default connect(
} );
},

onFocus( uid, config ) {
dispatch( {
type: 'UPDATE_FOCUS',
uid,
config,
} );
onFocus( ...args ) {
dispatch( focusBlock( ...args ) );
},

onRemove( uid ) {
Expand All @@ -321,12 +292,8 @@ export default connect(
} );
},

replaceBlocks( uids, blocks ) {
dispatch( {
type: 'REPLACE_BLOCKS',
uids,
blocks,
} );
onMerge( ...args ) {
mergeBlocks( dispatch, ...args );
},
} )
)( VisualEditorBlock );
5 changes: 5 additions & 0 deletions editor/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@ export function getPreviousBlock( state, uid ) {
return state.editor.blocksByUid[ state.editor.blockOrder[ order - 1 ] ] || null;
}

export function getNextBlock( state, uid ) {
const order = getBlockOrder( state, uid );
return state.editor.blocksByUid[ state.editor.blockOrder[ order + 1 ] ] || null;
}

export function isBlockSelected( state, uid ) {
return state.selectedBlock.uid === uid;
}
Expand Down
Loading