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

Try/deleting multi blocks #3194

Closed
wants to merge 4 commits into from
Closed

Try/deleting multi blocks #3194

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
14d2bcd
only pass through potential focus while not in multi-block selection
ephox-mogran Oct 27, 2017
40c6efd
focusing the next block after deleting
ephox-mogran Oct 27, 2017
96beac8
deleting and focusing blocks on BlockDelete button
ephox-mogran Oct 27, 2017
cc43e66
adding tests for DELETE_BLOCKS
ephox-mogran Oct 27, 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
16 changes: 16 additions & 0 deletions editor/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,6 +284,22 @@ export function removeBlocks( uids ) {
};
}

/**
* Returns an action object used in signalling that the blocks
* corresponding to the specified UID set are to be removed, and then
* another block given focus. It differs from removeBlocks in that it
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want a new action. Could we just use a flag?

Copy link
Member

Choose a reason for hiding this comment

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

Or dispatch two actions?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we shouldn't have two concurrent actions with hard-to-distinguish semantics.

In the new DELETE_BLOCKS effect, the removal itself is not conditional on state (only on payload, with the check for blockUids.length). Therefore, we should keep a single action, REMOVE_BLOCKS, and add a small effect for it that handles the new focus.

Now, the PR deals with two separate things:

  • Fixing deletion in multi-block selection
  • Focusing siblings after deletion

The former is a bug fix, the latter a UX improvement. I like it, but it begs the question: if we want to refocus a block after a multi-block deletion, are there any other cases (single-block deletion) where we don't want to refocus a block? Should we prefer a consistent behavior, whether that means always refocusing or never refocusing? cc @jasmussen

If still we want to make a distinction, then our effects.REMOVE_BLOCKS could look like:

if ( blockUids.length > 1 ) {
  const blockToFocus = …;
  dispatch( focusBlock( blockToFocus.uid ) );
}

Copy link
Contributor Author

@ephox-mogran ephox-mogran Oct 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Do we really want a new action. Could we just use a flag?

I've generally found flags are much harder to reason about later, and don't scale at all --- but that's my experience. I'm happy to go with the majority.

Or dispatch two actions?

I incorrectly assumed that for multi-actions, that effects is where they should go. I'm now realising that effects is for more complicated things. I don't think I'd seen many examples of where more than one action was dispatched in a component, so I thought that if you needed to do that, you put it in effects. My mistake.

are there any other cases (single-block deletion) where we don't want to refocus a block?

My rationale for keeping the old behaviour and adding new behaviour is that you never want to enforce focus in an action. You might find that something starts removing blocks asynchronously, and it can not focus grab from where you are. We could do some other logic, like only focus another block if we know for sure the focus was in one of the blocks to be removed (because nothing about the remove semantics enforces that they were selected), but I would strongly advise against always refocusing a block on remove unless it's a bit smarter than just "always".

In regards to the actions, effects separation, I'd really welcome something fixing it with a commit, and then I can look at the diff and see by example what your preferred approach is.

* calls focus on a block afterwards
*
* @param {String[]} uids Block UIDs
* @return {Object} Action object
*/
export function deleteBlocks( uids ) {
return {
type: 'DELETE_BLOCKS',
uids,
};
}

/**
* Returns an action object used in signalling that the block with the
* specified UID is to be removed.
Expand Down
4 changes: 2 additions & 2 deletions editor/block-settings-menu/block-delete-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { IconButton } from '@wordpress/components';
/**
* Internal dependencies
*/
import { removeBlocks } from '../actions';
import { deleteBlocks } from '../actions';

export function BlockDeleteButton( { onDelete, onClick = noop, small = false } ) {
const label = __( 'Delete' );
Expand All @@ -34,7 +34,7 @@ export default connect(
undefined,
( dispatch, ownProps ) => ( {
onDelete() {
dispatch( removeBlocks( ownProps.uids ) );
dispatch( deleteBlocks( ownProps.uids ) );
},
} )
)( BlockDeleteButton );
26 changes: 25 additions & 1 deletion editor/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import { BEGIN, COMMIT, REVERT } from 'redux-optimist';
import { get, uniqueId } from 'lodash';
import { get, uniqueId, first, last } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -22,6 +22,7 @@ import {
replaceBlocks,
createSuccessNotice,
createErrorNotice,
removeBlocks,
removeNotice,
savePost,
editPost,
Expand All @@ -33,6 +34,8 @@ import {
getDirtyMetaBoxes,
getEditedPostContent,
getPostEdits,
getPreviousBlock,
getNextBlock,
isCurrentPostPublished,
isEditedPostDirty,
isEditedPostNew,
Expand Down Expand Up @@ -184,6 +187,27 @@ export default {
const message = action.error.message && action.error.code !== 'unknown_error' ? action.error.message : __( 'Trashing failed' );
store.dispatch( createErrorNotice( message, { id: TRASH_POST_NOTICE_ID } ) );
},
DELETE_BLOCKS( action, store ) {
const { dispatch } = store;
const blockUids = action.uids;

if ( blockUids.length === 0 ) {
return;
}

const state = store.getState();
const firstBlock = first( blockUids );
const lastBlock = last( blockUids );
const priorBlock = getPreviousBlock( state, firstBlock );
const nextBlock = getNextBlock( state, lastBlock );

dispatch( removeBlocks( blockUids ) );

const nextFocus = nextBlock || priorBlock;
if ( nextFocus ) {
dispatch( focusBlock( nextFocus.uid ) );
}
},
MERGE_BLOCKS( action, store ) {
const { dispatch } = store;
const [ blockA, blockB ] = action.blocks;
Expand Down
5 changes: 3 additions & 2 deletions editor/modes/visual-editor/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,7 +290,7 @@ class VisualEditorBlock extends Component {
}

render() {
const { block, multiSelectedBlockUids, order, mode } = this.props;
const { block, multiSelectedBlockUids, order, mode, hasMultiSelection } = this.props;
const { name: blockName, isValid } = block;
const blockType = getBlockType( blockName );
// translators: %s: Type of block (i.e. Text, Image etc)
Expand Down Expand Up @@ -374,7 +374,7 @@ class VisualEditorBlock extends Component {
<BlockCrashBoundary onError={ this.onBlockError }>
{ isValid && mode === 'visual' && (
<BlockEdit
focus={ focus }
focus={ ! hasMultiSelection ? focus : null }
attributes={ block.attributes }
setAttributes={ this.setAttributes }
insertBlocksAfter={ this.insertBlocksAfter }
Expand Down Expand Up @@ -417,6 +417,7 @@ export default connect(
isSelected: isBlockSelected( state, ownProps.uid ),
isMultiSelected: isBlockMultiSelected( state, ownProps.uid ),
isFirstMultiSelected: isFirstMultiSelectedBlock( state, ownProps.uid ),
hasMultiSelection: getMultiSelectedBlockUids( state ).length > 1,
isHovered: isBlockHovered( state, ownProps.uid ),
focus: getBlockFocus( state, ownProps.uid ),
isSelecting: isMultiSelecting( state ),
Expand Down
8 changes: 4 additions & 4 deletions editor/modes/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ import PostTitle from '../../post-title';
import WritingFlow from '../../writing-flow';
import TableOfContents from '../../table-of-contents';
import { getBlockUids, getMultiSelectedBlockUids } from '../../selectors';
import { clearSelectedBlock, multiSelect, redo, undo, removeBlocks } from '../../actions';
import { clearSelectedBlock, multiSelect, redo, undo, deleteBlocks } from '../../actions';

class VisualEditor extends Component {
constructor() {
Expand Down Expand Up @@ -66,10 +66,10 @@ class VisualEditor extends Component {
}

deleteSelectedBlocks( event ) {
const { multiSelectedBlockUids, onRemove } = this.props;
const { multiSelectedBlockUids, onDelete } = this.props;
if ( multiSelectedBlockUids.length ) {
event.preventDefault();
onRemove( multiSelectedBlockUids );
onDelete( multiSelectedBlockUids );
}
}

Expand Down Expand Up @@ -116,6 +116,6 @@ export default connect(
onMultiSelect: multiSelect,
onRedo: redo,
onUndo: undo,
onRemove: removeBlocks,
onDelete: deleteBlocks,
}
)( VisualEditor );
73 changes: 73 additions & 0 deletions editor/test/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,9 @@ import {
resetPost,
setupNewPost,
mergeBlocks,
deleteBlocks,
focusBlock,
removeBlocks,
replaceBlocks,
editPost,
savePost,
Expand All @@ -31,6 +33,77 @@ describe( 'effects', () => {

beforeEach( () => jest.resetAllMocks() );

describe( '.DELETE_BLOCKS', () => {
const handler = effects.DELETE_BLOCKS;

it( 'No blocks to be deleted', () => {
selectors.getNextBlock.mockReturnValue( null );
selectors.getPreviousBlock.mockReturnValue( null );

const dispatch = jest.fn();
handler( deleteBlocks( [ ] ), { dispatch, getState: () => 'state' } );

expect( selectors.getPreviousBlock ).toHaveBeenCalledTimes( 0 );
expect( selectors.getNextBlock ).toHaveBeenCalledTimes( 0 );
expect( dispatch ).toHaveBeenCalledTimes( 0 );
} );

it( 'All blocks to be deleted', () => {
selectors.getNextBlock.mockReturnValue( null );
selectors.getPreviousBlock.mockReturnValue( null );

const dispatch = jest.fn();
handler( deleteBlocks( [ 'one', 'two', 'three' ] ), { dispatch, getState: () => 'state' } );

expect( selectors.getPreviousBlock ).toHaveBeenCalledWith( 'state', 'one' );
expect( selectors.getNextBlock ).toHaveBeenCalledWith( 'state', 'three' );
expect( dispatch ).toHaveBeenCalledTimes( 1 );
expect( dispatch ).toHaveBeenCalledWith( removeBlocks( [ 'one', 'two', 'three' ] ) );
} );

it( 'All blocks to be deleted but first', () => {
selectors.getNextBlock.mockReturnValue( { uid: 'first' } );
selectors.getPreviousBlock.mockReturnValue( null );

const dispatch = jest.fn();
handler( deleteBlocks( [ 'one', 'two', 'three' ] ), { dispatch, getState: () => 'state' } );

expect( selectors.getPreviousBlock ).toHaveBeenCalledWith( 'state', 'one' );
expect( selectors.getNextBlock ).toHaveBeenCalledWith( 'state', 'three' );
expect( dispatch ).toHaveBeenCalledTimes( 2 );
expect( dispatch ).toHaveBeenCalledWith( removeBlocks( [ 'one', 'two', 'three' ] ) );
expect( dispatch ).toHaveBeenCalledWith( focusBlock( 'first' ) );
} );

it( 'All blocks to be deleted but last', () => {
selectors.getNextBlock.mockReturnValue( null );
selectors.getPreviousBlock.mockReturnValue( { uid: 'last' } );

const dispatch = jest.fn();
handler( deleteBlocks( [ 'one', 'two', 'three' ] ), { dispatch, getState: () => 'state' } );

expect( dispatch ).toHaveBeenCalledTimes( 2 );
expect( selectors.getPreviousBlock ).toHaveBeenCalledWith( 'state', 'one' );
expect( selectors.getNextBlock ).toHaveBeenCalledWith( 'state', 'three' );
expect( dispatch ).toHaveBeenCalledWith( removeBlocks( [ 'one', 'two', 'three' ] ) );
expect( dispatch ).toHaveBeenCalledWith( focusBlock( 'last' ) );
} );

it( 'All blocks in the middle to be deleted', () => {
selectors.getNextBlock.mockReturnValue( { uid: 'first' } );
selectors.getPreviousBlock.mockReturnValue( { uid: 'last' } );

const dispatch = jest.fn();
handler( deleteBlocks( [ 'one', 'two', 'three' ] ), { dispatch, getState: () => 'state' } );

expect( dispatch ).toHaveBeenCalledTimes( 2 );
expect( selectors.getPreviousBlock ).toHaveBeenCalledWith( 'state', 'one' );
expect( selectors.getNextBlock ).toHaveBeenCalledWith( 'state', 'three' );
expect( dispatch ).toHaveBeenCalledWith( removeBlocks( [ 'one', 'two', 'three' ] ) );
expect( dispatch ).toHaveBeenCalledWith( focusBlock( 'first' ) );
} );
} );

describe( '.MERGE_BLOCKS', () => {
const handler = effects.MERGE_BLOCKS;

Expand Down