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

Try improving the side UI for nested blocks #5658

Merged
merged 12 commits into from
Apr 2, 2018
54 changes: 54 additions & 0 deletions blocks/library/columns/editor.scss
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
// These margins make sure that nested blocks stack/overlay with the parent block chrome
// This is sort of an experiment at making sure the editor looks as much like the end result as possible
// Potentially the rules here can apply to all nested blocks and enable stacking, in which case it should be moved elsewhere
.wp-block-columns .editor-block-list__layout {
&:first-child {
margin-left: -$block-padding;
}
&:last-child {
margin-right: -$block-padding;
}

// This max-width is used to constrain the main editor column, it should not cascade into columns
.editor-block-list__block {
max-width: none;
}
}

// Wide: show no left/right margin on wide, so they stack with the column side UI
.editor-block-list__block[data-align="wide"] .wp-block-columns .editor-block-list__layout {
&:first-child {
margin-left: 0;
}
&:last-child {
margin-right: 0;
}
}

// Fullwide: show margin left/right to ensure there's room for the side UI
// This is not a 1:1 preview with the front-end where these margins would presumably be zero
// @todo this could be revisited, by for example showing this margin only when the parent block was selected first
// Then at least an unselected columns block would be an accurate preview
.editor-block-list__block[data-align="full"] .wp-block-columns .editor-block-list__layout {
&:first-child {
margin-left: $block-side-ui-padding;
}
&:last-child {
margin-right: $block-side-ui-padding;
}
}

// Hide appender shortcuts in columns
// @todo This essentially duplicates the mobile styles for the appender component
// It would be nice to be able to use element queries in that component instead https://github.com/tomhodgins/element-queries-spec
.wp-block-columns {
.editor-inserter-with-shortcuts {
display: none;
}

.editor-block-list__empty-block-inserter,
.editor-default-block-appender .editor-inserter {
left: auto;
right: $item-spacing;
}
}
1 change: 1 addition & 0 deletions blocks/library/columns/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import { RangeControl } from '@wordpress/components';
* Internal dependencies
*/
import './style.scss';
import './editor.scss';
import InspectorControls from '../../inspector-controls';
import BlockControls from '../../block-controls';
import BlockAlignmentToolbar from '../../block-alignment-toolbar';
Expand Down
5 changes: 5 additions & 0 deletions blocks/library/paragraph/editor.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@
background: white;
}

// Don't show white background when a nesting parent is selected
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning for the white background in the first place?

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 actually can't recall. Hmm. @youknowriad?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it was introduced here: 775428e#diff-c2685bc3733aece65a4ed211668982e0R2

So it's probably related to the ability to colorize paragraph backgrounds. But removing it doesn't seem to make a difference. @jorgefilipecosta any thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

Seems we shouldn't want to assume a white background, especially if the plan is to support themes to define their own editor styles.

Copy link
Member

Choose a reason for hiding this comment

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

The reason why we set a white background is related to the contrast verification. If the background was never set getComputedStyles returns back background (if I remember correctly). So adding a background fixes the problem. If themes change the background it also works well because as long as a background is set we are able to retrieve it. The problem happens when no background is set. As we get back background color when background is not set we cannot differentiate and assume is white background because in fact it may be set to black. I will try to research this in more detail maybe there is a way to detect when the background is not set and in that case assume white.
Replicate from comment #5273 (comment).

Copy link
Member

Choose a reason for hiding this comment

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

But this only works if the style is explicitly set, i.e. a theme would need to target and override the .editor-block-list__block:not( .is-multi-selected ) .wp-block-paragraph selector (or selector of equivalent specificity). And the contrast checker would otherwise falsely report contrast issues on the basis of a white background? i.e. if theme actually had black background where a white text would be very contrasting, it would be reported by the editor as an issue.

I'm wondering if it's really worth the trouble to assume a default value (vs. disabling until a background color is known / set).

Alternatively, could we recurse up through parent elements until we find one where background color is not the default value?

Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove the background color, for now, as it is causing us troubles so we can advance.
After, I really like the idea, of recursively iterating up to the parent elements. And I will try to explore it. If we find it not a good solution than we disable contrast checker for colors not explicitly set.

.editor-block-list__layout .editor-block-list__layout .editor-block-list__block .wp-block-paragraph {
background: inherit;
}

.blocks-font-size__main {
display: flex;
justify-content: space-between;
Expand Down
1 change: 1 addition & 0 deletions edit-post/assets/stylesheets/_variables.scss
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ $block-padding: 14px;
$block-mover-margin: 18px;
$block-spacing: 4px;
$block-side-ui-padding: 36px;
$block-side-ui-width: 28px; // The side UI max height matches a single line of text, 56px. 28px is half, allowing 2 mover arrows

// Buttons & UI Widgets
$button-style__radius-roundrect: 4px;
Expand Down
4 changes: 2 additions & 2 deletions edit-post/components/visual-editor/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,8 @@
margin-right: -$block-side-ui-padding;
}

&[data-align="full"] .editor-block-contextual-toolbar,
&[data-align="wide"] .editor-block-contextual-toolbar {
&[data-align="full"] > .editor-block-contextual-toolbar,
&[data-align="wide"] > .editor-block-contextual-toolbar { // don't affect nested block toolbars
max-width: $content-width + 2; // 1px border left and right
margin-left: auto;
margin-right: auto;
Expand Down
23 changes: 13 additions & 10 deletions editor/components/block-list/block.js
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ import InvalidBlockWarning from './invalid-block-warning';
import BlockCrashWarning from './block-crash-warning';
import BlockCrashBoundary from './block-crash-boundary';
import BlockHtml from './block-html';
import BlockBreadcrumb from './breadcrumb';
import BlockContextualToolbar from './block-contextual-toolbar';
import BlockMultiControls from './multi-controls';
import BlockMobileToolbar from './block-mobile-toolbar';
Expand Down Expand Up @@ -438,11 +439,11 @@ export class BlockListBlock extends Component {

// 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 last block in the top-level list rendering.
// is the first block in the top-level list rendering.
const shouldShowInsertionPoint = (
( ! isMultiSelected && ! isLast ) ||
( ! isMultiSelected && ! isFirst ) ||
( isMultiSelected && isLastInSelection ) ||
( isLast && ! rootUID && ! isEmptyDefaultBlock )
( isFirst && ! rootUID && ! isEmptyDefaultBlock )
);

// Generate the wrapper class names handling the different states of the block.
Expand Down Expand Up @@ -479,6 +480,7 @@ export class BlockListBlock extends Component {
<IgnoreNestedEvents
ref={ this.setBlockListRef }
onMouseOver={ this.maybeHover }
onMouseOverHandled={ this.hideHoverEffects }
onMouseLeave={ this.hideHoverEffects }
className={ wrapperClassName }
data-type={ block.name }
Expand All @@ -493,6 +495,13 @@ export class BlockListBlock extends Component {
] }
{ ...wrapperProps }
>
{ shouldShowInsertionPoint && (
<BlockInsertionPoint
uid={ block.uid }
rootUID={ rootUID }
layout={ layout }
/>
) }
<BlockDropZone
index={ order }
rootUID={ rootUID }
Expand All @@ -514,6 +523,7 @@ export class BlockListBlock extends Component {
renderBlockMenu={ renderBlockMenu }
/>
) }
{ isHovered && <BlockBreadcrumb uid={ block.uid } /> }
{ shouldShowContextualToolbar && <BlockContextualToolbar /> }
{ isFirstMultiSelected && <BlockMultiControls rootUID={ rootUID } /> }
<IgnoreNestedEvents
Expand Down Expand Up @@ -561,13 +571,6 @@ export class BlockListBlock extends Component {
) }
</IgnoreNestedEvents>
{ !! error && <BlockCrashWarning /> }
{ shouldShowInsertionPoint && (
<BlockInsertionPoint
uid={ block.uid }
rootUID={ rootUID }
layout={ layout }
/>
) }
{ showSideInserter && (
<Fragment>
<div className="editor-block-list__side-inserter">
Expand Down
77 changes: 77 additions & 0 deletions editor/components/block-list/breadcrumb.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
/**
* WordPress dependencies
*/
import { compose } from '@wordpress/element';
import { Dashicon, Tooltip, Toolbar, Button } from '@wordpress/components';
import { withDispatch, withSelect } from '@wordpress/data';
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
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
* the root block.
*
* @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 default compose( [
withSelect( ( select, ownProps ) => {
const { getBlockRootUID } = select( 'core/editor' );
const { uid } = ownProps;

return {
rootUID: getBlockRootUID( uid ),
};
} ),
withDispatch( ( dispatch, ownProps ) => {
const { rootUID } = ownProps;
const { selectBlock } = dispatch( 'core/editor' );

return {
selectRootBlock: () => selectBlock( rootUID ),
};
} ),
] )( BlockBreadcrumb );
28 changes: 20 additions & 8 deletions editor/components/block-list/ignore-nested-events.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,10 +43,7 @@ class IgnoreNestedEvents extends Component {
* @return {void}
*/
proxyEvent( event ) {
// Skip if already handled (i.e. assume nested block)
if ( event.nativeEvent._blockHandled ) {
return;
}
const isHandled = !! event.nativeEvent._blockHandled;

// Assign into the native event, since React will reuse their synthetic
// event objects and this property assignment could otherwise leak.
Expand All @@ -55,7 +52,14 @@ class IgnoreNestedEvents extends Component {
event.nativeEvent._blockHandled = true;

// Invoke original prop handler
const propKey = this.eventMap[ event.type ];
let propKey = this.eventMap[ event.type ];

// If already handled (i.e. assume nested block), only invoke a
// corresponding "Handled"-suffixed prop callback.
if ( isHandled ) {
propKey += 'Handled';
}

if ( this.props[ propKey ] ) {
this.props[ propKey ]( event );
}
Expand All @@ -69,16 +73,24 @@ class IgnoreNestedEvents extends Component {
...Object.keys( props ),
], ( result, key ) => {
// Try to match prop key as event handler
const match = key.match( /^on([A-Z][a-zA-Z]+)$/ );
const match = key.match( /^on([A-Z][a-zA-Z]+?)(Handled)?$/ );
if ( match ) {
const isHandledProp = !! match[ 2 ];
if ( isHandledProp ) {
// Avoid assigning through the invalid prop key. This
// assumes mutation of shallow clone by above spread.
delete props[ key ];
}

// Re-map the prop to the local proxy handler to check whether
// the event has already been handled.
result[ key ] = this.proxyEvent;
const proxiedPropName = 'on' + match[ 1 ];
result[ proxiedPropName ] = this.proxyEvent;

// Assign event -> propName into an instance variable, so as to
// avoid re-renders which could be incurred either by setState
// or in mapping values to a newly created function.
this.eventMap[ match[ 1 ].toLowerCase() ] = key;
this.eventMap[ match[ 1 ].toLowerCase() ] = proxiedPropName;
}

return result;
Expand Down
2 changes: 1 addition & 1 deletion editor/components/block-list/insertion-point.js
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ export default compose(
connect(
( state, { uid, rootUID } ) => {
const blockIndex = uid ? getBlockIndex( state, uid, rootUID ) : -1;
const insertIndex = blockIndex + 1;
const insertIndex = blockIndex;
const insertionPoint = getBlockInsertionPoint( state );
const block = uid ? getBlock( state, uid ) : null;

Expand Down
2 changes: 0 additions & 2 deletions editor/components/block-list/layout.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,6 @@ import { Component } from '@wordpress/element';
*/
import './style.scss';
import BlockListBlock from './block';
import BlockInsertionPoint from './insertion-point';
import IgnoreNestedEvents from './ignore-nested-events';
import DefaultBlockAppender from '../default-block-appender';
import {
Expand Down Expand Up @@ -216,7 +215,6 @@ class BlockListLayout extends Component {

return (
<div className={ classes }>
{ !! blockUIDs.length && <BlockInsertionPoint rootUID={ rootUID } layout={ defaultLayout } /> }
{ map( blockUIDs, ( uid, blockIndex ) => (
<BlockListBlock
key={ 'block-' + uid }
Expand Down
Loading