-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Changes from all commits
4d5fa38
63e4c81
fc2efa4
3d4a22d
bf2731f
667905e
f614065
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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'; | ||
|
@@ -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 | ||
|
@@ -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( { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this needed? There was a problem hiding this comment. Choose a reason for hiding this commentThe 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( [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -699,3 +699,11 @@ | |
padding-top: 6px; | ||
} | ||
} | ||
|
||
.editor-block-breadcrumb { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aside: I applied an invalid class name per standards; this should be |
||
opacity: 0; | ||
|
||
&.is-visible { | ||
@include fade_in; | ||
} | ||
} |
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 ) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we have used |
||
class WithHoverAreasComponent extends Component { | ||
constructor() { | ||
super( ...arguments ); | ||
this.state = { | ||
hoverArea: null, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can easily add an |
||
}; | ||
this.onMouseLeave = this.onMouseLeave.bind( this ); | ||
this.onMouseMove = this.onMouseMove.bind( this ); | ||
} | ||
|
||
componentDidMount() { | ||
// Disable reason: We use findDOMNode to avoid unnecessary extra dom Nodes | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this the main reason for using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, the main reason is that There was a problem hiding this comment. Choose a reason for hiding this commentThe 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). 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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
https://gist.github.com/paulirish/5d52fb081b3570c81e3a And in a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess I can look for alternatives to There was a problem hiding this comment. Choose a reason for hiding this commentThe 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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. :)