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: Try adding the left/right block hover areas #6101

Merged
merged 7 commits into from
Apr 12, 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
5 changes: 4 additions & 1 deletion edit-post/assets/stylesheets/_animations.scss
Original file line number Diff line number Diff line change
Expand Up @@ -17,4 +17,7 @@
animation: slide_in_right 0.1s forwards;
}


@mixin fade_in {
animation: fade-in 0.3s ease-out;
animation-fill-mode: forwards;
}
9 changes: 9 additions & 0 deletions edit-post/assets/stylesheets/main.scss
Original file line number Diff line number Diff line change
Expand Up @@ -147,3 +147,12 @@ body.gutenberg-editor-page {
padding: 6px 10px;
}
}

@keyframes fade-in {
from {
opacity: 0;
}
to {
opacity: 1;
}
}
20 changes: 14 additions & 6 deletions editor/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import BlockDraggable from './block-draggable';
import IgnoreNestedEvents from './ignore-nested-events';
import InserterWithShortcuts from '../inserter-with-shortcuts';
import Inserter from '../inserter';
import withHoverAreas from './with-hover-areas';
import { createInnerBlockList } from '../../utils/block-list';

const { BACKSPACE, DELETE, ENTER } = keycodes;
Expand Down Expand Up @@ -404,8 +405,10 @@ export class BlockListBlock extends Component {
isFirstMultiSelected,
isLastInSelection,
isTypingWithinBlock,
isMultiSelecting,
hoverArea,
} = this.props;
const isHovered = this.state.isHovered && ! this.props.isMultiSelecting;
const isHovered = this.state.isHovered && ! isMultiSelecting;
const { name: blockName, isValid } = block;
const blockType = getBlockType( blockName );
// translators: %s: Type of block (i.e. Text, Image etc)
Expand All @@ -419,8 +422,10 @@ export class BlockListBlock extends Component {
const isSelectedNotTyping = isSelected && ! isTypingWithinBlock;
const showSideInserter = ( isSelected || isHovered ) && isEmptyDefaultBlock;
const shouldAppearSelected = ! showSideInserter && isSelectedNotTyping;
const shouldShowMovers = ( shouldAppearSelected || isHovered || ( isEmptyDefaultBlock && isSelectedNotTyping ) ) && ! showSideInserter;
const shouldShowSettingsMenu = shouldShowMovers;
// We render block movers and block settings to keep them tabbale even if hidden
const shouldRenderMovers = ( isSelected || hoverArea === 'left' ) && ! showSideInserter && ! isMultiSelecting && ! isMultiSelected;
const shouldRenderBlockSettings = ( isSelected || hoverArea === 'right' ) && ! showSideInserter && ! isMultiSelecting && ! isMultiSelected;
const shouldShowBreadcrumb = isHovered;
const shouldShowContextualToolbar = shouldAppearSelected && isValid && showContextualToolbar;
const shouldShowMobileToolbar = shouldAppearSelected;
const { error, dragging } = this.state;
Expand Down Expand Up @@ -510,23 +515,25 @@ export class BlockListBlock extends Component {
rootUID={ rootUID }
layout={ layout }
/>
{ shouldShowMovers && (
{ shouldRenderMovers && (
<BlockMover
uids={ [ uid ] }
rootUID={ rootUID }
layout={ layout }
isFirst={ isFirst }
isLast={ isLast }
isHidden={ ! isHovered || hoverArea !== 'left' }
/>
) }
{ shouldShowSettingsMenu && ! showSideInserter && (
{ shouldRenderBlockSettings && (
<BlockSettingsMenu
uids={ [ uid ] }
rootUID={ rootUID }
renderBlockMenu={ renderBlockMenu }
isHidden={ ! isHovered || hoverArea !== 'right' }
Copy link
Member

Choose a reason for hiding this comment

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

@youknowriad Why does the toolbar only hide? Why not prevent rendering? If there's a reason, it's not immediately clear from this code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it should stay tabbable. When it receives the focus it's shown.
There's a comment above (with a typo :P)

We render block movers and block settings to keep them tabbale even if hidden

Copy link
Member

Choose a reason for hiding this comment

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

Oh, thanks, I didn't read that far up. :)

/>
) }
{ isHovered && <BlockBreadcrumb uid={ uid } /> }
{ shouldShowBreadcrumb && <BlockBreadcrumb uid={ uid } isHidden={ hoverArea !== 'left' } /> }
{ shouldShowContextualToolbar && <BlockContextualToolbar /> }
{ isFirstMultiSelected && <BlockMultiControls rootUID={ rootUID } /> }
<IgnoreNestedEvents
Expand Down Expand Up @@ -692,4 +699,5 @@ export default compose(
};
} ),
withFilters( 'editor.BlockListBlock' ),
withHoverAreas
)( BlockListBlock );
93 changes: 58 additions & 35 deletions editor/components/block-list/breadcrumb.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
/**
* External dependencies
*/
import classnames from 'classnames';

/**
* WordPress dependencies
*/
import { compose } from '@wordpress/element';
import { compose, Component } from '@wordpress/element';
import { Dashicon, Tooltip, Toolbar, Button } from '@wordpress/components';
import { withDispatch, withSelect } from '@wordpress/data';
import { __ } from '@wordpress/i18n';
Expand All @@ -12,20 +17,6 @@ import { __ } from '@wordpress/i18n';
import NavigableToolbar from '../navigable-toolbar';
import BlockTitle from '../block-title';

/**
* Stops propagation of the given event argument. Assumes that the event has
* been completely handled and no other listeners should be informed.
*
* For the breadcrumb component, this is used for improved interoperability
* with the block's `onFocus` handler which selects the block, thus conflicting
* with the intention to select the root block.
*
* @param {Event} event Event for which propagation should be stopped.
*/
function stopPropagation( event ) {
event.stopPropagation();
}

/**
* Block breadcrumb component, displaying the label of the block. If the block
* descends from a root block, a button is displayed enabling the user to select
Expand All @@ -34,27 +25,59 @@ function stopPropagation( event ) {
* @param {string} props.uid UID of block.
* @param {string} props.rootUID UID of block's root.
* @param {Function} props.selectRootBlock Callback to select root block.
*
* @return {WPElement} Breadcrumb element.
*/
function BlockBreadcrumb( { uid, rootUID, selectRootBlock } ) {
return (
<NavigableToolbar className="editor-block-breadcrumb">
<Toolbar>
{ rootUID && (
<Tooltip text={ __( 'Select parent block' ) }>
<Button
onClick={ selectRootBlock }
onFocus={ stopPropagation }
>
<Dashicon icon="arrow-left" uid={ uid } />
</Button>
</Tooltip>
) }
<BlockTitle uid={ uid } />
</Toolbar>
</NavigableToolbar>
);
export class BlockBreadcrumb extends Component {
constructor() {
super( ...arguments );
this.state = {
isFocused: false,
};
this.onFocus = this.onFocus.bind( this );
this.onBlur = this.onBlur.bind( this );
}

onFocus( event ) {
this.setState( {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In the breadcrumb component at the moment no because it's not shown at all when the block is selected, but I didn't want to write the implementation depending on something not in the component itself. If tomorrow we decide to show this breadcrumb even on selected blocks, this will become necessary.

isFocused: true,
} );

// This is used for improved interoperability
// with the block's `onFocus` handler which selects the block, thus conflicting
// with the intention to select the root block.
event.stopPropagation();
}

onBlur() {
this.setState( {
isFocused: false,
} );
}

render( ) {
const { uid, rootUID, selectRootBlock, isHidden } = this.props;
const { isFocused } = this.state;

return (
<NavigableToolbar className={ classnames( 'editor-block-breadcrumb', {
'is-visible': ! isHidden || isFocused,
} ) }>
<Toolbar>
{ rootUID && (
<Tooltip text={ __( 'Select parent block' ) }>
<Button
onClick={ selectRootBlock }
onFocus={ this.onFocus }
onBlur={ this.onBlur }
>
<Dashicon icon="arrow-left" uid={ uid } />
</Button>
</Tooltip>
) }
<BlockTitle uid={ uid } />
</Toolbar>
</NavigableToolbar>
);
}
}

export default compose( [
Expand Down
8 changes: 8 additions & 0 deletions editor/components/block-list/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -699,3 +699,11 @@
padding-top: 6px;
}
}

.editor-block-breadcrumb {
Copy link
Member

Choose a reason for hiding this comment

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

Aside: I applied an invalid class name per standards; this should be .editor-block-list__breadcrumb

opacity: 0;

&.is-visible {
@include fade_in;
}
}
62 changes: 62 additions & 0 deletions editor/components/block-list/with-hover-areas.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
/**
* WordPress dependencies
*/
import { Component, findDOMNode } from '@wordpress/element';

const withHoverAreas = ( WrappedComponent ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Should we have used @wordpress/element's createHigherOrderComponent ?

class WithHoverAreasComponent extends Component {
constructor() {
super( ...arguments );
this.state = {
hoverArea: null,
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 can easily add an isHovered state to this component to remove it from the block.js component but it seems that this state is slightly more complex than just keeping a boolean on whether the block is hovered or not. Might be good to do but I'll defer to @aduth here. I think you have more knowledge about the edge cases there.

};
this.onMouseLeave = this.onMouseLeave.bind( this );
this.onMouseMove = this.onMouseMove.bind( this );
}

componentDidMount() {
// Disable reason: We use findDOMNode to avoid unnecessary extra dom Nodes
Copy link
Member

Choose a reason for hiding this comment

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

Was this the main reason for using mousemove instead of some floating invisible elements which trigger on mouseenter?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the main reason is that mouseenter doesn't trigger on areas with lower z-index, since these areas would need a lower z-index to allow writing in the editables etc...

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I see what you mean. I played with some ideas for creating an inverted hover area "hole" to trigger the unhover while still allowing the contents to be interacted, but it's certainly more complex. Maybe worth exploring, but I struggled to get it working perfectly (particularly for the "hole" to be interacted with, and capturing all unhovers with various layering contexts).

invert-hover

Diff: https://gist.github.com/aduth/d08fe307491f641f333fcd362ef38bf5

// eslint-disable-next-line react/no-find-dom-node
this.container = findDOMNode( this );
this.container.addEventListener( 'mousemove', this.onMouseMove );
this.container.addEventListener( 'mouseleave', this.onMouseLeave );
}

componentWillUnmount() {
this.container.removeEventListener( 'mousemove', this.onMouseMove );
this.container.removeEventListener( 'mouseleave', this.onMouseLeave );
}

onMouseLeave() {
if ( this.state.hoverArea ) {
this.setState( { hoverArea: null } );
}
}

onMouseMove( event ) {
const { width, left, right } = this.container.getBoundingClientRect();
Copy link
Member

Choose a reason for hiding this comment

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

What forces layout / reflow

[...] This is also called reflow or layout thrashing, and is common performance bottleneck.

Box metrics

  • [...] elem.getBoundingClientRect()

https://gist.github.com/paulirish/5d52fb081b3570c81e3a

And in a mousemove, it's likely being called hundreds of times 🙁

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 guess I can look for alternatives to getBoundingClientRect

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can't think of any alternative without browser reflow


let hoverArea = null;
if ( ( event.clientX - left ) < width / 3 ) {
hoverArea = 'left';
} else if ( ( right - event.clientX ) < width / 3 ) {
hoverArea = 'right';
}

if ( hoverArea !== this.state.hoverArea ) {
this.setState( { hoverArea } );
}
}

render() {
const { hoverArea } = this.state;
return (
<WrappedComponent { ...this.props } hoverArea={ hoverArea } />
);
}
}

return WithHoverAreasComponent;
};

export default withHoverAreas;
Loading