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: Display inserter button after block on hover, focus #2890

Merged
merged 4 commits into from
Oct 9, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
16 changes: 16 additions & 0 deletions components/dropdown/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,22 @@ class Dropdown extends Component {
};
}

componentWillUnmount() {
const { isOpen } = this.state;
const { onToggle } = this.props;
if ( isOpen && onToggle ) {
onToggle( false );
}
}

componentWillUpdate( nextProps, nextState ) {
const { isOpen } = nextState;
const { onToggle } = nextProps;
if ( this.state.isOpen !== isOpen && onToggle ) {
onToggle( isOpen );
}
}

bindContainer( ref ) {
this.container = ref;
}
Expand Down
26 changes: 26 additions & 0 deletions editor/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,32 @@ export function hideInsertionPoint() {
};
}

/**
* Returns an action object used in signalling that block insertion should
* occur at the specified block index position.
*
* @param {Number} position Position at which to insert
* @return {Object} Action object
*/
export function setBlockInsertionPoint( position ) {
return {
type: 'SET_BLOCK_INSERTION_POINT',
position,
};
}

/**
* Returns an action object used in signalling that the block insertion point
* should be reset.
*
* @return {Object} Action object
*/
export function clearBlockInsertionPoint() {
return {
type: 'CLEAR_BLOCK_INSERTION_POINT',
};
}

export function editPost( edits ) {
return {
type: 'EDIT_POST',
Expand Down
13 changes: 7 additions & 6 deletions editor/assets/stylesheets/_z-index.scss
Original file line number Diff line number Diff line change
Expand Up @@ -6,20 +6,21 @@ $z-layers: (
'.editor-block-switcher__arrow': 1,
'.editor-visual-editor__block:before': -1,
'.editor-visual-editor__block .wp-block-more:before': -1,
'.editor-visual-editor__block {core/image aligned left or right}': 10,
'.editor-block-toolbar': 1,
'.editor-visual-editor__block {core/image aligned left or right}': 20,
'.editor-block-toolbar': 10,
'.editor-visual-editor__block-warning': 1,
'.editor-visual-editor .editor-visual-editor__block .editor-inserter': 1,
'.components-form-toggle__input': 1,
'.editor-format-list__menu': 1,
'.editor-inserter__tabs': 1,
'.editor-inserter__tab.is-active': 1,
'.components-panel__header': 1,
'.blocks-format-toolbar__link-modal': 1,
'.editor-block-switcher__menu': 2,
'.editor-block-mover': 10,
'.blocks-gallery-image__inline-menu': 10,
'.editor-header': 20,
'.editor-text-editor__formatting': 20,
'.editor-block-mover': 20,
'.blocks-gallery-image__inline-menu': 20,
'.editor-header': 30,
'.editor-text-editor__formatting': 30,

// Show drop zone above most standard content, but below any overlays
'.components-drop-zone': 100,
Expand Down
103 changes: 73 additions & 30 deletions editor/inserter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,52 +2,91 @@
* External dependencies
*/
import { connect } from 'react-redux';
import { bindActionCreators } from 'redux';

/**
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { Dropdown, IconButton } from '@wordpress/components';
import { createBlock } from '@wordpress/blocks';
import { Component } from '@wordpress/element';

/**
* Internal dependencies
*/
import InserterMenu from './menu';
import { getBlockInsertionPoint, getEditorMode } from '../selectors';
import { insertBlock, hideInsertionPoint } from '../actions';
import {
insertBlock,
setBlockInsertionPoint,
clearBlockInsertionPoint,
hideInsertionPoint,
} from '../actions';

function Inserter( { position, children, onInsertBlock, insertionPoint } ) {
return (
<Dropdown
className="editor-inserter"
position={ position }
renderToggle={ ( { onToggle, isOpen } ) => (
<IconButton
icon="insert"
label={ __( 'Insert block' ) }
onClick={ onToggle }
className="editor-inserter__toggle"
aria-haspopup="true"
aria-expanded={ isOpen }
>
{ children }
</IconButton>
) }
renderContent={ ( { onClose } ) => {
const onInsert = ( name ) => {
onInsertBlock(
name,
insertionPoint
);
class Inserter extends Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess we don't need this to be a class component, but probably the same discussion for/against inline props.

Copy link
Member

Choose a reason for hiding this comment

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

I hope we will see the day where there is only one supported way of creating components in React. I feel like some components are converted back and forth whenever requirements change.

constructor() {
super( ...arguments );

onClose();
};
this.toggleInsertionPoint = this.toggleInsertionPoint.bind( this );
}

return <InserterMenu onSelect={ onInsert } />;
} }
/>
);
toggleInsertionPoint( isOpen ) {
const {
insertIndex,
setInsertionPoint,
clearInsertionPoint,
} = this.props;

if ( insertIndex !== undefined ) {
if ( isOpen ) {
setInsertionPoint( insertIndex );
} else {
clearInsertionPoint();
}
}
}

render() {
const {
position,
children,
onInsertBlock,
insertionPoint,
} = this.props;

return (
<Dropdown
className="editor-inserter"
position={ position }
onToggle={ this.toggleInsertionPoint }
renderToggle={ ( { onToggle, isOpen } ) => (
<IconButton
icon="insert"
label={ __( 'Insert block' ) }
onClick={ onToggle }
className="editor-inserter__toggle"
aria-haspopup="true"
aria-expanded={ isOpen }
>
{ children }
</IconButton>
) }
renderContent={ ( { onClose } ) => {
const onInsert = ( name ) => {
onInsertBlock(
name,
insertionPoint
);

onClose();
};

return <InserterMenu onSelect={ onInsert } />;
} }
/>
);
}
}

export default connect(
Expand All @@ -65,5 +104,9 @@ export default connect(
position
) );
},
...bindActionCreators( {
setInsertionPoint: setBlockInsertionPoint,
clearInsertionPoint: clearBlockInsertionPoint,
}, dispatch ),
} )
)( Inserter );
20 changes: 12 additions & 8 deletions editor/inserter/menu.js
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ export class InserterMenu extends Component {
}

componentDidMount() {
document.addEventListener( 'keydown', this.onKeyDown );
document.addEventListener( 'keydown', this.onKeyDown, true );
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why this change is necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a similar issue to the one discussed at #2706: Namely, WritingFlow is handling the arrow presses and moving between blocks. The true argument allows us to handle the key press earlier during the capture phase. I might take another pass at this, and I wonder why we need to bind to the document at all instead of using React events (since I expect bubbling would work better in the latter case). If I keep it as-is, I'll add a clarifying comment.

}

componentWillUnmount() {
document.removeEventListener( 'keydown', this.onKeyDown );
document.removeEventListener( 'keydown', this.onKeyDown, true );
}

componentDidUpdate( prevProps, prevState ) {
Expand Down Expand Up @@ -239,28 +239,32 @@ export class InserterMenu extends Component {
return;
}
this.focusPrevious( this );

break;

case UP:
keydown.preventDefault();
this.focusPrevious( this );

break;

case RIGHT:
if ( this.state.currentFocus === 'search' ) {
return;
}
this.focusNext( this );

break;

case DOWN:
keydown.preventDefault();
this.focusNext( this );

break;
default :
break;

default:
return;
}

// Since unhandled key will return in the default case, we can assume
// having reached this point implies that the key is handled.
keydown.stopImmediatePropagation();
}

changeMenuSelection( refName ) {
Expand Down
23 changes: 18 additions & 5 deletions editor/modes/visual-editor/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import BlockDropZone from './block-drop-zone';
import BlockMover from '../../block-mover';
import BlockRightMenu from '../../block-settings-menu';
import BlockToolbar from '../../block-toolbar';
import Inserter from '../../inserter';
import {
clearSelectedBlock,
editPost,
Expand All @@ -49,6 +50,7 @@ import {
isBlockSelected,
isFirstMultiSelectedBlock,
isTyping,
isInsertingSiblingBlock,
} from '../../selectors';

const { BACKSPACE, ESCAPE, DELETE, ENTER } = keycodes;
Expand All @@ -61,6 +63,7 @@ class VisualEditorBlock extends Component {
this.setAttributes = this.setAttributes.bind( this );
this.maybeHover = this.maybeHover.bind( this );
this.maybeStartTyping = this.maybeStartTyping.bind( this );
this.mouseLeaveUnlessInserting = this.mouseLeaveUnlessInserting.bind( this );
this.stopTypingOnMouseMove = this.stopTypingOnMouseMove.bind( this );
this.removeOrDeselect = this.removeOrDeselect.bind( this );
this.mergeBlocks = this.mergeBlocks.bind( this );
Expand Down Expand Up @@ -157,9 +160,9 @@ class VisualEditorBlock extends Component {
}

maybeHover() {
const { isHovered, isSelected, isMultiSelected, onHover } = this.props;
const { isHovered, isSelected, isMultiSelected, isInserterOpen, onHover } = this.props;

if ( isHovered || isSelected || isMultiSelected ) {
if ( isHovered || isSelected || isMultiSelected || isInserterOpen ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this check is to avoid "removing" the inserter when we hover out of the block?
Maybe we can handle this in a simpler way: select the block when we open its inserter.
I'm just trying to avoid this dependency between the inserter and this component.

Copy link
Member Author

Choose a reason for hiding this comment

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

Maybe we can handle this in a simpler way: select the block when we open its inserter.

That sounds like a good idea! I'll give it a try.

return;
}

Expand All @@ -177,6 +180,12 @@ class VisualEditorBlock extends Component {
}
}

mouseLeaveUnlessInserting() {
if ( ! this.props.isInserterOpen ) {
this.props.onMouseLeave();
}
}

stopTypingOnMouseMove( { clientX, clientY } ) {
const { lastClientX, lastClientY } = this;

Expand Down Expand Up @@ -278,7 +287,7 @@ class VisualEditorBlock extends Component {
}

render() {
const { block, multiSelectedBlockUids, order } = this.props;
const { block, multiSelectedBlockUids, order, nextBlock } = 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 @@ -310,7 +319,7 @@ class VisualEditorBlock extends Component {
'is-hovered': isHovered,
} );

const { onMouseLeave, onFocus, onReplace } = this.props;
const { onFocus, onReplace } = this.props;

// Determine whether the block has props to apply to the wrapper.
let wrapperProps;
Expand All @@ -331,7 +340,7 @@ class VisualEditorBlock extends Component {
onFocus={ this.onFocus }
onMouseMove={ this.maybeHover }
onMouseEnter={ this.maybeHover }
onMouseLeave={ onMouseLeave }
onMouseLeave={ this.mouseLeaveUnlessInserting }
className={ wrapperClassname }
data-type={ block.name }
tabIndex="0"
Expand Down Expand Up @@ -381,6 +390,9 @@ class VisualEditorBlock extends Component {
}
</BlockCrashBoundary>
</div>
{ ( showUI || isHovered ) && !! nextBlock && (
<Inserter insertIndex={ order + 1 } />
) }
{ !! error && <BlockCrashWarning /> }
</div>
);
Expand All @@ -403,6 +415,7 @@ export default connect(
order: getBlockIndex( state, ownProps.uid ),
multiSelectedBlockUids: getMultiSelectedBlockUids( state ),
meta: getEditedPostAttribute( state, 'meta' ),
isInserterOpen: isInsertingSiblingBlock( state ),
};
},
( dispatch, ownProps ) => ( {
Expand Down
20 changes: 20 additions & 0 deletions editor/modes/visual-editor/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -281,6 +281,26 @@
border-bottom: 3px solid $blue-medium-500;
}
}

.editor-visual-editor & .editor-inserter {
position: absolute;
left: $block-padding + $block-mover-padding-visible;
top: 100%;
transform: translate( -50%, -50% );
margin-top: 0;
margin-bottom: 0;
z-index: z-index( '.editor-visual-editor .editor-visual-editor__block .editor-inserter' );
}

.editor-visual-editor & .editor-inserter__toggle {
padding: 4px;
background-color: white;

&,
& .dashicon {
display: block;
}
}
}

.editor-visual-editor .editor-inserter {
Expand Down
Loading