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

Block: Avoid rerendering when the previous/next block updates #5025

Merged
merged 2 commits into from
Feb 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
18 changes: 9 additions & 9 deletions editor/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ import {
isMultiSelecting,
getBlockIndex,
getEditedPostAttribute,
getNextBlock,
getPreviousBlock,
getNextBlockUid,
Copy link
Member

Choose a reason for hiding this comment

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

We need to come to a convention on camel-case for abbreviations. Elsewhere I've used e.g. rootUID

Related: #2511

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right! I thought we kind of agreed on always camel casing abbreviations at some point but happy to change anyway.

getPreviousBlockUid,
isBlockHovered,
isBlockMultiSelected,
isBlockSelected,
Expand Down Expand Up @@ -290,20 +290,20 @@ export class BlockListBlock extends Component {
}

mergeBlocks( forward = false ) {
const { block, previousBlock, nextBlock, onMerge } = this.props;
const { block, previousBlockUid, nextBlockUid, onMerge } = this.props;

// Do nothing when it's the first block.
if (
( ! forward && ! previousBlock ) ||
( forward && ! nextBlock )
( ! forward && ! previousBlockUid ) ||
( forward && ! nextBlockUid )
) {
return;
}

if ( forward ) {
onMerge( block, nextBlock );
onMerge( block.uid, nextBlockUid );
} else {
onMerge( previousBlock, block );
onMerge( previousBlockUid, block.uid );
}

// Manually trigger typing mode, since merging will remove this block and
Expand Down Expand Up @@ -612,8 +612,8 @@ export class BlockListBlock extends Component {
const mapStateToProps = ( state, { uid, rootUID } ) => {
const isSelected = isBlockSelected( state, uid );
return {
previousBlock: getPreviousBlock( state, uid ),
nextBlock: getNextBlock( state, uid ),
previousBlockUid: getPreviousBlockUid( state, uid ),
nextBlockUid: getNextBlockUid( state, uid ),
block: getBlock( state, uid ),
isMultiSelected: isBlockMultiSelected( state, uid ),
isFirstMultiSelected: isFirstMultiSelectedBlock( state, uid ),
Expand Down
24 changes: 12 additions & 12 deletions editor/components/writing-flow/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ import {
placeCaretAtVerticalEdge,
} from '../../utils/dom';
import {
getPreviousBlock,
getNextBlock,
getPreviousBlockUid,
getNextBlockUid,
getMultiSelectedBlocksStartUid,
getMultiSelectedBlocks,
getSelectedBlock,
Expand Down Expand Up @@ -155,20 +155,20 @@ class WritingFlow extends Component {
}

expandSelection( currentStartUid, isReverse ) {
const { previousBlock, nextBlock } = this.props;
const { previousBlockUid, nextBlockUid } = this.props;

const expandedBlock = isReverse ? previousBlock : nextBlock;
if ( expandedBlock ) {
this.props.onMultiSelect( currentStartUid, expandedBlock.uid );
const expandedBlockUid = isReverse ? previousBlockUid : nextBlockUid;
if ( expandedBlockUid ) {
this.props.onMultiSelect( currentStartUid, expandedBlockUid );
}
}

moveSelection( isReverse ) {
const { previousBlock, nextBlock } = this.props;
const { previousBlockUid, nextBlockUid } = this.props;

const focusedBlock = isReverse ? previousBlock : nextBlock;
if ( focusedBlock ) {
this.props.onSelectBlock( focusedBlock.uid );
const focusedBlockUid = isReverse ? previousBlockUid : nextBlockUid;
if ( focusedBlockUid ) {
this.props.onSelectBlock( focusedBlockUid );
}
}

Expand Down Expand Up @@ -299,8 +299,8 @@ class WritingFlow extends Component {

export default connect(
( state ) => ( {
previousBlock: getPreviousBlock( state ),
nextBlock: getNextBlock( state ),
previousBlockUid: getPreviousBlockUid( state ),
nextBlockUid: getNextBlockUid( state ),
selectionStart: getMultiSelectedBlocksStartUid( state ),
hasMultiSelection: getMultiSelectedBlocks( state ).length > 1,
selectedBlock: getSelectedBlock( state ),
Expand Down
12 changes: 10 additions & 2 deletions editor/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -257,10 +257,18 @@ export function trashPost( postId, postType ) {
};
}

export function mergeBlocks( blockA, blockB ) {
/**
* Returns an action object used in signalling that two blocks should be merged
*
* @param {string} blockAUid UID of the first block to merge.
* @param {string} blockBUid UID of the second block to merge.
*
* @return {Object} Action object.
*/
export function mergeBlocks( blockAUid, blockBUid ) {
return {
type: 'MERGE_BLOCKS',
blocks: [ blockA, blockB ],
blocks: [ blockAUid, blockBUid ],
};
}

Expand Down
5 changes: 4 additions & 1 deletion editor/store/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,10 @@ export default {
},
MERGE_BLOCKS( action, store ) {
const { dispatch } = store;
const [ blockA, blockB ] = action.blocks;
const state = store.getState();
const [ blockAUid, blockBUid ] = action.blocks;
const blockA = getBlock( state, blockAUid );
const blockB = getBlock( state, blockBUid );
const blockType = getBlockType( blockA.name );

// Only focus the previous block if it's not mergeable
Expand Down
24 changes: 12 additions & 12 deletions editor/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -538,17 +538,17 @@ export function getBlockRootUID( state, uid ) {
}

/**
* Returns the block adjacent one at the given reference startUID and modifier
* Returns the UID of the block adjacent one at the given reference startUID and modifier
* directionality. Defaults start UID to the selected block, and direction as
* next block. Returns null if there is no adjacent block.
*
* @param {Object} state Global application state.
* @param {?string} startUID Optional UID of block from which to search.
* @param {?number} modifier Directionality multiplier (1 next, -1 previous).
*
* @return {?Object} Adjacent block object, or null if none exists.
* @return {?string} Return the UID of the block, or null if none exists.
*/
export function getAdjacentBlock( state, startUID, modifier = 1 ) {
export function getAdjacentBlockUid( state, startUID, modifier = 1 ) {
// Default to selected block.
if ( startUID === undefined ) {
startUID = get( getSelectedBlock( state ), 'uid' );
Expand Down Expand Up @@ -591,33 +591,33 @@ export function getAdjacentBlock( state, startUID, modifier = 1 ) {
}

// Assume incremented index is within the set.
return getBlock( state, orderSet[ nextIndex ] );
return orderSet[ nextIndex ];
}

/**
* Returns the previous block from the given reference startUID. Defaults start
* Returns the previous block's UID from the given reference startUID. Defaults start
* UID to the selected block. Returns null if there is no previous block.
*
* @param {Object} state Global application state.
* @param {?string} startUID Optional UID of block from which to search.
*
* @return {?Object} Adjacent block object, or null if none exists.
* @return {?string} Adjacent block's UID, or null if none exists.
*/
export function getPreviousBlock( state, startUID ) {
return getAdjacentBlock( state, startUID, -1 );
export function getPreviousBlockUid( state, startUID ) {
return getAdjacentBlockUid( state, startUID, -1 );
}

/**
* Returns the next block from the given reference startUID. Defaults start UID
* Returns the next block's UID from the given reference startUID. Defaults start UID
* to the selected block. Returns null if there is no next block.
*
* @param {Object} state Global application state.
* @param {?string} startUID Optional UID of block from which to search.
*
* @return {?Object} Adjacent block object, or null if none exists.
* @return {?string} Adjacent block's UID, or null if none exists.
*/
export function getNextBlock( state, startUID ) {
return getAdjacentBlock( state, startUID, 1 );
export function getNextBlockUid( state, startUID ) {
return getAdjacentBlockUid( state, startUID, 1 );
}

/**
Expand Down
12 changes: 4 additions & 8 deletions editor/store/test/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -272,15 +272,11 @@ describe( 'actions', () => {

describe( 'mergeBlocks', () => {
it( 'should return MERGE_BLOCKS action', () => {
const blockA = {
uid: 'blockA',
};
const blockB = {
uid: 'blockB',
};
expect( mergeBlocks( blockA, blockB ) ).toEqual( {
const blockAUid = 'blockA';
const blockBUid = 'blockB';
expect( mergeBlocks( blockAUid, blockBUid ) ).toEqual( {
type: 'MERGE_BLOCKS',
blocks: [ blockA, blockB ],
blocks: [ blockAUid, blockBUid ],
} );
} );
} );
Expand Down
27 changes: 23 additions & 4 deletions editor/store/test/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,13 @@ describe( 'effects', () => {

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

afterEach( () => {
getBlockTypes().forEach( ( block ) => {
unregisterBlockType( block.name );
} );
selectors.getBlock = defaultGetBlock;
} );

it( 'should only focus the blockA if the blockA has no merge function', () => {
Expand All @@ -65,8 +67,13 @@ describe( 'effects', () => {
uid: 'ribs',
name: 'core/test-block',
};
selectors.getBlock = ( state, uid ) => {
return blockA.uid === uid ? blockA : blockB;
};

const dispatch = jest.fn();
handler( mergeBlocks( blockA, blockB ), { dispatch } );
const getState = () => ( {} );
handler( mergeBlocks( blockA.uid, blockB.uid ), { dispatch, getState } );

expect( dispatch ).toHaveBeenCalledTimes( 1 );
expect( dispatch ).toHaveBeenCalledWith( selectBlock( 'chicken' ) );
Expand All @@ -93,8 +100,12 @@ describe( 'effects', () => {
name: 'core/test-block',
attributes: { content: 'ribs' },
};
selectors.getBlock = ( state, uid ) => {
return blockA.uid === uid ? blockA : blockB;
};
const dispatch = jest.fn();
handler( mergeBlocks( blockA, blockB ), { dispatch } );
const getState = () => ( {} );
handler( mergeBlocks( blockA.uid, blockB.uid ), { dispatch, getState } );

expect( dispatch ).toHaveBeenCalledTimes( 2 );
expect( dispatch ).toHaveBeenCalledWith( selectBlock( 'chicken', -1 ) );
Expand Down Expand Up @@ -127,8 +138,12 @@ describe( 'effects', () => {
name: 'core/test-block2',
attributes: { content: 'ribs' },
};
selectors.getBlock = ( state, uid ) => {
return blockA.uid === uid ? blockA : blockB;
};
const dispatch = jest.fn();
handler( mergeBlocks( blockA, blockB ), { dispatch } );
const getState = () => ( {} );
handler( mergeBlocks( blockA.uid, blockB.uid ), { dispatch, getState } );

expect( dispatch ).not.toHaveBeenCalled();
} );
Expand Down Expand Up @@ -180,8 +195,12 @@ describe( 'effects', () => {
name: 'core/test-block-2',
attributes: { content2: 'ribs' },
};
selectors.getBlock = ( state, uid ) => {
return blockA.uid === uid ? blockA : blockB;
};
const dispatch = jest.fn();
handler( mergeBlocks( blockA, blockB ), { dispatch } );
const getState = () => ( {} );
handler( mergeBlocks( blockA.uid, blockB.uid ), { dispatch, getState } );

expect( dispatch ).toHaveBeenCalledTimes( 2 );
// expect( dispatch ).toHaveBeenCalledWith( focusBlock( 'chicken', { offset: -1 } ) );
Expand Down
Loading