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 List: Use default Inserter for sibling insertion #11018

Merged
merged 4 commits into from
Oct 30, 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
10 changes: 8 additions & 2 deletions docs/data/data-core-editor.md
Original file line number Diff line number Diff line change
Expand Up @@ -707,7 +707,7 @@ otherwise.

*Returns*

Whether block is first in mult-selection.
Whether block is first in multi-selection.

### isBlockMultiSelected

Expand Down Expand Up @@ -1534,14 +1534,20 @@ be inserted, optionally at a specific index respective a root block list.

* blocks: Block objects to insert.
* index: Index at which block should be inserted.
* rootClientId: Optional root cliente ID of block list on
* rootClientId: Optional root client ID of block list on
which to insert.

### showInsertionPoint

Returns an action object used in signalling that the insertion point should
be shown.

*Parameters*

* rootClientId: Optional root client ID of block list on
which to insert.
* index: Index at which block should be inserted.

### hideInsertionPoint

Returns an action object hiding the insertion point.
Expand Down
8 changes: 3 additions & 5 deletions packages/editor/src/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -412,10 +412,9 @@ export class BlockListBlock extends Component {
const shouldShowMobileToolbar = shouldAppearSelected;
const { error, dragging } = this.state;

// Insertion point can only be made visible when the side inserter is
// not present, and either the block is at the extent of a selection or
// is the first block in the top-level list rendering.
const shouldShowInsertionPoint = ( isPartOfMultiSelection && isFirst ) || ! isPartOfMultiSelection;
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was wrong before; isFirst means first of an entire rootClientId context, not first in a multi-selection.

// Insertion point can only be made visible if the block is at the
// the extent of a multi-selection, or not in a multi-selection.
const shouldShowInsertionPoint = ( isPartOfMultiSelection && isFirstMultiSelected ) || ! isPartOfMultiSelection;
const canShowInBetweenInserter = ! isEmptyDefaultBlock && ! isPreviousBlockADefaultEmptyBlock;

// The wp-block className is important for editor styles.
Expand Down Expand Up @@ -501,7 +500,6 @@ export class BlockListBlock extends Component {
clientId={ clientId }
rootClientId={ rootClientId }
canShowInserter={ canShowInBetweenInserter }
onInsert={ this.hideHoverEffects }
/>
) }
<BlockDropZone
Expand Down
118 changes: 50 additions & 68 deletions packages/editor/src/components/block-list/insertion-point.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { isUnmodifiedDefaultBlock } from '@wordpress/blocks';
import { Component } from '@wordpress/element';
import { IconButton } from '@wordpress/components';
import { withSelect, withDispatch } from '@wordpress/data';
import { ifCondition, compose } from '@wordpress/compose';
import { withSelect } from '@wordpress/data';

/**
* Internal dependencies
*/
import Inserter from '../inserter';

class BlockInsertionPoint extends Component {
constructor() {
Expand All @@ -22,12 +24,12 @@ class BlockInsertionPoint extends Component {

this.onBlurInserter = this.onBlurInserter.bind( this );
this.onFocusInserter = this.onFocusInserter.bind( this );
this.onClick = this.onClick.bind( this );
}

onFocusInserter( event ) {
// We stop propagation of the focus event to avoid selecting the current block
// While we're trying to insert a new block
// Stop propagation of the focus event to avoid selecting the current
// block while inserting a new block, as it is not relevant to sibling
// insertion and conflicts with contextual toolbar placement.
event.stopPropagation();

this.setState( {
Expand All @@ -41,77 +43,57 @@ class BlockInsertionPoint extends Component {
} );
}

onClick() {
const { rootClientId, index, ...props } = this.props;
props.insertDefaultBlock( undefined, rootClientId, index );
props.startTyping();
this.onBlurInserter();
if ( props.onInsert ) {
this.props.onInsert();
}
}

render() {
const { isInserterFocused } = this.state;
const { showInsertionPoint, showInserter } = this.props;
const {
showInsertionPoint,
canShowInserter,
rootClientId,
insertIndex,
} = this.props;

return (
<div className="editor-block-list__insertion-point">
{ showInsertionPoint && <div className="editor-block-list__insertion-point-indicator" /> }
{ showInserter && (
<div className={ classnames( 'editor-block-list__insertion-point-inserter', { 'is-visible': isInserterFocused } ) }>
<IconButton
icon="insert"
className="editor-block-list__insertion-point-button"
onClick={ this.onClick }
label={ __( 'Insert block' ) }
onFocus={ this.onFocusInserter }
onBlur={ this.onBlurInserter }
{ showInsertionPoint && (
<div className="editor-block-list__insertion-point-indicator" />
) }
{ canShowInserter && (
<div
onFocus={ this.onFocusInserter }
onBlur={ this.onBlurInserter }
className={
classnames( 'editor-block-list__insertion-point-inserter', {
'is-visible': isInserterFocused,
} )
}
>
<Inserter
rootClientId={ rootClientId }
index={ insertIndex }
/>
</div>
) }
</div>
);
}
}
export default compose(
withSelect( ( select, { clientId, rootClientId, canShowInserter } ) => {
const {
canInsertBlockType,
getBlockIndex,
getBlockInsertionPoint,
getBlock,
isBlockInsertionPointVisible,
isTyping,
} = select( 'core/editor' );
const {
getDefaultBlockName,
} = select( 'core/blocks' );
const blockIndex = clientId ? getBlockIndex( clientId, rootClientId ) : -1;
Copy link
Member Author

Choose a reason for hiding this comment

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

The clientId behavior was totally broken anyways, and unused. Became obvious as I was trying to implement it for the "before first block" by rendering as first child of the BlockList.

const insertIndex = blockIndex;
const insertionPoint = getBlockInsertionPoint();
const block = clientId ? getBlock( clientId ) : null;
const showInsertionPoint = (
isBlockInsertionPointVisible() &&
insertionPoint.index === insertIndex &&
insertionPoint.rootClientId === rootClientId &&
( ! block || ! isUnmodifiedDefaultBlock( block ) )
);
export default withSelect( ( select, { clientId, rootClientId } ) => {
const {
getBlockIndex,
getBlockInsertionPoint,
getBlock,
isBlockInsertionPointVisible,
} = select( 'core/editor' );
const blockIndex = getBlockIndex( clientId, rootClientId );
const insertIndex = blockIndex;
const insertionPoint = getBlockInsertionPoint();
const block = getBlock( clientId );
const showInsertionPoint = (
isBlockInsertionPointVisible() &&
insertionPoint.index === insertIndex &&
insertionPoint.rootClientId === rootClientId &&
! isUnmodifiedDefaultBlock( block )
);

const defaultBlockName = getDefaultBlockName();
return {
canInsertDefaultBlock: canInsertBlockType( defaultBlockName, rootClientId ),
showInserter: ! isTyping() && canShowInserter,
index: insertIndex,
showInsertionPoint,
};
} ),
ifCondition( ( { canInsertDefaultBlock } ) => canInsertDefaultBlock ),
withDispatch( ( dispatch ) => {
const { insertDefaultBlock, startTyping } = dispatch( 'core/editor' );
return {
insertDefaultBlock,
startTyping,
};
} )
)( BlockInsertionPoint );
return { showInsertionPoint, insertIndex };
} )( BlockInsertionPoint );
4 changes: 2 additions & 2 deletions packages/editor/src/components/block-list/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,7 @@
justify-content: center;

// Show a clickable plus.
.editor-block-list__insertion-point-button {
.editor-inserter__toggle {
margin-top: -4px;
border-radius: 50%;
color: $blue-medium-focus;
Expand All @@ -666,7 +666,7 @@
// Don't show the sibling inserter before the selected block.
.edit-post-layout:not(.has-fixed-toolbar) {
// The child selector is necessary for this to work properly in nested contexts.
.is-selected > .editor-block-list__insertion-point > .editor-block-list__insertion-point-inserter {
.is-selected > .editor-block-list__insertion-point .editor-inserter__toggle {
opacity: 0;
pointer-events: none;

Expand Down
5 changes: 2 additions & 3 deletions packages/editor/src/components/inserter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,15 +99,14 @@ class Inserter extends Component {
}

export default compose( [
withSelect( ( select, { rootClientId } ) => {
withSelect( ( select, { rootClientId, index } ) => {
const {
getEditedPostAttribute,
getBlockInsertionPoint,
getInserterItems,
} = select( 'core/editor' );

let index;
if ( rootClientId === undefined ) {
if ( rootClientId === undefined && index === undefined ) {
// Unless explicitly provided, the default insertion point provided
// by the store occurs immediately following the selected block.
// Otherwise, the default behavior for an undefined index is to
Expand Down
6 changes: 4 additions & 2 deletions packages/editor/src/components/inserter/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -130,10 +130,12 @@ export class InserterMenu extends Component {
hoveredItem: item,
} );

const { showInsertionPoint, hideInsertionPoint } = this.props;
if ( item ) {
this.props.showInsertionPoint();
const { rootClientId, index } = this.props;
showInsertionPoint( rootClientId, index );
} else {
this.props.hideInsertionPoint();
hideInsertionPoint();
}
}

Expand Down
10 changes: 8 additions & 2 deletions packages/editor/src/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -312,7 +312,7 @@ export function insertBlock( block, index, rootClientId ) {
*
* @param {Object[]} blocks Block objects to insert.
* @param {?number} index Index at which block should be inserted.
* @param {?string} rootClientId Optional root cliente ID of block list on
* @param {?string} rootClientId Optional root client ID of block list on
* which to insert.
*
* @return {Object} Action object.
Expand All @@ -331,11 +331,17 @@ export function insertBlocks( blocks, index, rootClientId ) {
* Returns an action object used in signalling that the insertion point should
* be shown.
*
* @param {?string} rootClientId Optional root client ID of block list on
* which to insert.
* @param {?number} index Index at which block should be inserted.
*
* @return {Object} Action object.
*/
export function showInsertionPoint() {
export function showInsertionPoint( rootClientId, index ) {
return {
type: 'SHOW_INSERTION_POINT',
rootClientId,
index,
};
}

Expand Down
14 changes: 8 additions & 6 deletions packages/editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -710,21 +710,23 @@ export function blocksMode( state = {}, action ) {
}

/**
* Reducer returning the block insertion point visibility, a boolean value
* reflecting whether the insertion point should be shown.
* Reducer returning the block insertion point visibility, either null if there
* is not an explicit insertion point assigned, or an object of its `index` and
* `rootClientId`.
*
* @param {Object} state Current state.
* @param {Object} action Dispatched action.
*
* @return {Object} Updated state.
*/
export function isInsertionPointVisible( state = false, action ) {
export function insertionPoint( state = null, action ) {
switch ( action.type ) {
case 'SHOW_INSERTION_POINT':
return true;
const { rootClientId, index } = action;
return { rootClientId, index };

case 'HIDE_INSERTION_POINT':
return false;
return null;
}

return state;
Expand Down Expand Up @@ -1100,7 +1102,7 @@ export default optimist( combineReducers( {
blockSelection,
blocksMode,
blockListSettings,
isInsertionPointVisible,
insertionPoint,
preferences,
saving,
postLock,
Expand Down
15 changes: 10 additions & 5 deletions packages/editor/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -1016,10 +1016,10 @@ export function getLastMultiSelectedBlockClientId( state ) {
* specified client ID is the first block of the multi-selection set, or false
* otherwise.
*
* @param {Object} state Editor state.
* @param {string} clientId Block client ID.
* @param {Object} state Editor state.
* @param {string} clientId Block client ID.
*
* @return {boolean} Whether block is first in mult-selection.
* @return {boolean} Whether block is first in multi-selection.
*/
export function isFirstMultiSelectedBlock( state, clientId ) {
return getFirstMultiSelectedBlockClientId( state ) === clientId;
Expand Down Expand Up @@ -1277,7 +1277,12 @@ export function isCaretWithinFormattedText( state ) {
export function getBlockInsertionPoint( state ) {
let rootClientId, index;

const { end } = state.blockSelection;
const { insertionPoint, blockSelection } = state;
if ( insertionPoint !== null ) {
return insertionPoint;
}

const { end } = blockSelection;
if ( end ) {
rootClientId = getBlockRootClientId( state, end ) || undefined;
index = getBlockIndex( state, end, rootClientId ) + 1;
Expand All @@ -1296,7 +1301,7 @@ export function getBlockInsertionPoint( state ) {
* @return {?boolean} Whether the insertion point is visible or not.
*/
export function isBlockInsertionPointVisible( state ) {
return state.isInsertionPointVisible;
return state.insertionPoint !== null;
}

/**
Expand Down
Loading