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.

Sign up for GitHub

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

Adds the block hierarchy navigation menu to the header #10545

Merged
merged 32 commits into from
Oct 17, 2018
Merged

Adds the block hierarchy navigation menu to the header #10545

Show file tree
Hide file tree
Changes from 30 commits
Commits
Show all changes
32 commits
Select commit Hold shift + click to select a range
fceb875
Adds the block hierarchy navivation menu to the header
youknowriad Oct 12, 2018
8eee332
Avoid auto-selecting inner blocks when selecting the parent block
youknowriad Oct 15, 2018
94a20dd
Show block hierarchy for container blocks
youknowriad Oct 15, 2018
a110a3b
highlight the selected block
youknowriad Oct 15, 2018
be61554
Add a unit test for the newly added selector
youknowriad Oct 15, 2018
b1bbd5e
Adding an e2e test
youknowriad Oct 15, 2018
4fcaff2
Replace block names by block titles
youknowriad Oct 15, 2018
e1b402c
Fix unit tests
youknowriad Oct 15, 2018
3d4c28f
Fix e2e tests
youknowriad Oct 15, 2018
5779576
Update the selector docs
youknowriad Oct 15, 2018
7ea0064
Tweak the design of the block navigation menu
youknowriad Oct 15, 2018
ba32da1
Compress the block hierarchy tree
youknowriad Oct 15, 2018
083d2df
Update inner column block icon.
mtias Oct 15, 2018
b22412a
Avoid showing the parent block's borders if a child block is selected
youknowriad Oct 15, 2018
5fc0693
Simplify the menu's label
youknowriad Oct 15, 2018
b67a487
Tweak the selected block's style
youknowriad Oct 15, 2018
d55686c
Darker text color for the selected block
youknowriad Oct 15, 2018
e8af488
Memoize the block parent's selectors
youknowriad Oct 15, 2018
a014263
Adding root level block navigation
youknowriad Oct 16, 2018
e1fea28
Adding a keyboard shortcut to open the navigation menu
youknowriad Oct 16, 2018
ca407ed
Avoid wrapping function descriptions
youknowriad Oct 16, 2018
c81b4a7
Use Path for a mobile-friendly SVG
youknowriad Oct 16, 2018
affb3b3
Extract inner blocks check to a dom utility
youknowriad Oct 16, 2018
67e8451
Fix unit tests
youknowriad Oct 16, 2018
b6099fa
Fix e2e tests
youknowriad Oct 16, 2018
0ad245e
Add aria-label to block navigation list to improve context with keybo…
tofumatt Oct 16, 2018
9909857
Use the label prop to label the MenuGroup
youknowriad Oct 16, 2018
e70e5fb
Use access + b shortcut to avoid ambiguity
youknowriad Oct 16, 2018
82394d3
Add role="presentation" to the items that should be invisible a11y wise
youknowriad Oct 16, 2018
2b796a7
Fix unit tests
youknowriad Oct 16, 2018
a1c039d
change shortcuts, improve tests, and show empty navigator when no blo…
tofumatt Oct 17, 2018
45b7686
Tweak tests; use o instead of l
tofumatt Oct 17, 2018
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
13 changes: 13 additions & 0 deletions docs/data/data-core-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -565,6 +565,19 @@ exist.

Root client ID, if exists

### getBlockHierarchyRootClientId

Given a block client ID, returns the root of the hierarchy from which the block is nested, return the block itself for root level blocks.

*Parameters*

* state: Editor state.
* clientId: Block from which to find root client ID.

*Returns*

Root client ID

### getAdjacentBlockClientId

Returns the client ID of the block adjacent one at the given reference
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/columns/column.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ export const settings = {

parent: [ 'core/columns' ],

icon: 'columns',
icon: <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24"><path fill="none" d="M0 0h24v24H0V0z" /><path d="M11.99 18.54l-7.37-5.73L3 14.07l9 7 9-7-1.63-1.27zM12 16l7.36-5.73L21 9l-9-7-9 7 1.63 1.27L12 16zm0-11.47L17.74 9 12 13.47 6.26 9 12 4.53z" /></svg>,

description: __( 'A single column within a columns block.' ),

Expand Down
5 changes: 4 additions & 1 deletion packages/components/src/menu-group/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,10 @@ export function MenuGroup( {
}

const labelId = `components-menu-group-label-${ instanceId }`;
const classNames = classnames( className, 'components-menu-group' );
const classNames = classnames(
className,
'components-menu-group'
);

return (
<div className={ classNames }>
Expand Down
18 changes: 1 addition & 17 deletions packages/components/src/menu-item/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
width: 100%;
padding: 8px;
text-align: left;
padding-left: 2rem;
color: $dark-gray-500;

// Target plugin icons that can have arbitrary classes by using an aggressive selector.
Expand All @@ -18,10 +17,6 @@
flex: 0 0 auto;
}

&.has-icon {
padding-left: 0.5rem;
}

&:hover:not(:disabled):not([aria-disabled="true"]) {
@include menu-style__hover;
}
Expand All @@ -30,18 +25,6 @@
@include menu-style__focus;
}

// Colorize plugin icons to ensure contrast and cohesion, but allow plugin developers to override.
svg,
svg * {
fill: $dark-gray-500;
}

&:hover svg,
&:hover svg * {
// !important allows icons from plugins to be overriden and given a dark-gray fill
fill: $dark-gray-900 !important;
}

// Don't wrap text until viewport is beyond the mobile breakpoint.
@include break-mobile() {
.components-popover:not(.is-mobile) & {
Expand All @@ -65,3 +48,4 @@
display: inline;
}
}

2 changes: 2 additions & 0 deletions packages/edit-post/src/components/header/header-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
EditorHistoryRedo,
EditorHistoryUndo,
NavigableToolbar,
BlockNavigationDropdown,
} from '@wordpress/editor';

/**
Expand All @@ -36,6 +37,7 @@ function HeaderToolbar( { hasFixedToolbar, isLargeViewport, mode } ) {
<EditorHistoryUndo />
<EditorHistoryRedo />
<TableOfContents />
<BlockNavigationDropdown />
{ hasFixedToolbar && isLargeViewport && (
<div className="edit-post-header-toolbar__block-toolbar">
<BlockToolbar />
Expand Down
8 changes: 8 additions & 0 deletions packages/edit-post/src/components/header/more-menu/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,12 @@
> div:not(:last-child) .components-menu-group {
border-bottom: $border-width solid $light-gray-500;
}

.components-menu-item__button {
padding-left: 2rem;

&.has-icon {
padding-left: 0.5rem;
}
}
}
4 changes: 4 additions & 0 deletions packages/edit-post/src/components/keyboard-shortcut-help-modal/config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,10 @@ const globalShortcuts = {
description: __( 'Show or hide the settings sidebar.' ),
ariaLabel: shortcutAriaLabel.primaryShift( ',' ),
},
{
keyCombination: access( 'b' ),
Copy link
Member

Choose a reason for hiding this comment

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

This combination opens the bookmarks menu in Firefox on Windows, so it won't work. I'll find one that does and edit the PR. 😓

Copy link
Member

Choose a reason for hiding this comment

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

Looks like access( 'l' ) will work across browsers and OSes that I've tested (Edge + Firefox/Chrome on Windows/Mac). I'll change it to that for now.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll just mention what I posted in slack here. I started compiling a list of browser shortcuts in a spreadsheet a while ago:
https://docs.google.com/spreadsheets/d/1nK1frKawxV7aboWOJbbslbIqBGoLY7gqKvfwqENj2yE/edit#gid=0

I note that we had access( 'l' ) earmarked for Align Left. How about access( 'o' ), as in O for outline?

description: __( 'Open the block navigation menu.' ),
},
{
keyCombination: ctrl( '`' ),
description: __( 'Navigate to the next part of the editor.' ),
Expand Down
10 changes: 10 additions & 0 deletions ...es/edit-post/src/components/keyboard-shortcut-help-modal/test/__snapshots__/index.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,16 @@ exports[`KeyboardShortcutHelpModal should match snapshot when the modal is activ
",",
],
},
Object {
"description": "Open the block navigation menu.",
"keyCombination": Array [
"Shift",
"+",
"Alt",
"+",
"B",
],
},
Object {
"ariaLabel": "Control + Backtick",
"description": "Navigate to the next part of the editor.",
Expand Down
20 changes: 10 additions & 10 deletions packages/editor/src/components/block-list/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ import IgnoreNestedEvents from '../ignore-nested-events';
import InserterWithShortcuts from '../inserter-with-shortcuts';
import Inserter from '../inserter';
import withHoverAreas from './with-hover-areas';
import { isInsideRootBlock } from '../../utils/dom';

export class BlockListBlock extends Component {
constructor() {
Expand Down Expand Up @@ -91,7 +92,7 @@ export class BlockListBlock extends Component {
}

if ( this.props.isSelected && ! prevProps.isSelected ) {
this.focusTabbable();
this.focusTabbable( true );
Copy link
Member

Choose a reason for hiding this comment

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

What is it we're trying to do here? And why is it inconsistent with the behavior in componentDidMount?

Copy link
Member

Choose a reason for hiding this comment

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

Okay, so if it's to avoid "select" actually causing one of its inner blocks to become selected, can't this also happen in a newly-mounted block as well?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should reconsider whether to call focusTabbable on a selection update as well. I don't recall what we need it for.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should reconsider whether to call focusTabbable on a selection update as well. I don't recall what we need it for.

Initial implementation here was to avoid selecting innerBlocks in all cases but this breaks the writing flow when inserting columns (it actually broke 2 e2e tests) because the first paragraph was not focused in that case. I did consider whether it's fine to not focus the first paragraph in that case as assume we hit "tab" or click no the appender of the column block first but I think from a "Writing flow" perspective, it's better if we focus the first paragraph in a column directly when creating a columns block.

Copy link
Member

Choose a reason for hiding this comment

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

As "Columns" evolve, it'd be natural to focus on the container when it is added rather than on the paragraph within.

Copy link
Member

Choose a reason for hiding this comment

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

it's better if we focus the first paragraph in a column directly when creating a columns block.

Personally I've found this behavior to be a weird / unexpected side effect of focus behaviors. On the Layout<->Writing spectrum, Columns falls more on the former, so I don't necessarily see the paragraph creation / focus being a necessity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As "Columns" evolve, it'd be natural to focus on the container when it is added rather than on the paragraph within.

Yes one idea I had was to rewrite the columns block similarly to the table block where you select the number of columns before typing but I though this was out of the scope of this PR anyway.

Copy link
Member

Choose a reason for hiding this comment

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

I'm getting a sense we're becoming very ad hoc with how and when we expect focus transitions to occur upon block selection, hacking in various conditions to preserve intended flows while losing sight of what those intended flows are. It's signalling to me that naively checking for / comparing on isSelected from BlockListBlock is not serving us very well, and that perhaps it ought to be moved / refactored to more appropriately capture the user interactions where we expect it to occur: as best I can tell, this is mostly in splitting / merging / replacing actions occurring while typing within a RichText.

Copy link
Member

Choose a reason for hiding this comment

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

For columns I think the right way is to not focus the inner block. In general, I don't see a reason for focusing the inner block since when you add a container (columns, section, etc) you are likely to want to customize the container first. Pushing you to a paragraph seems detrimental unless it's a block like "Quote".

}
}

Expand All @@ -118,8 +119,10 @@ export class BlockListBlock extends Component {

/**
* When a block becomes selected, transition focus to an inner tabbable.
*
* @param {boolean} ignoreInnerBlocks Should not focus inner blocks.
*/
focusTabbable() {
focusTabbable( ignoreInnerBlocks ) {
const { initialPosition } = this.props;

// Focus is captured by the wrapper node, so while focus transition
Expand All @@ -131,7 +134,11 @@ export class BlockListBlock extends Component {
}

// Find all tabbables within node.
const textInputs = focus.tabbable.find( this.node ).filter( isTextField );
const textInputs = focus.tabbable
.find( this.node )
.filter( isTextField )
// Exclude inner blocks
.filter( ( node ) => ! ignoreInnerBlocks || isInsideRootBlock( this.node, node ) );

// If reversed (e.g. merge via backspace), use the last in the set of
// tabbables.
Expand Down Expand Up @@ -370,9 +377,7 @@ export class BlockListBlock extends Component {
isEmptyDefaultBlock,
isMovable,
isPreviousBlockADefaultEmptyBlock,
hasSelectedInnerBlock,
isParentOfSelectedBlock,
hasMultiSelection,
isDraggable,
} = this.props;
const isHovered = this.state.isHovered && ! isMultiSelecting;
Expand All @@ -390,7 +395,6 @@ export class BlockListBlock extends Component {
const showEmptyBlockSideInserter = ( isSelected || isHovered ) && isEmptyDefaultBlock && isValid;
const showSideInserter = ( isSelected || isHovered ) && isEmptyDefaultBlock;
const shouldAppearSelected = ! isFocusMode && ! hasFixedToolbar && ! showSideInserter && isSelected && ! isTypingWithinBlock;
const shouldAppearSelectedParent = ! isFocusMode && ! hasFixedToolbar && ! showSideInserter && hasSelectedInnerBlock && ! isTypingWithinBlock && ! hasMultiSelection;
const shouldAppearHovered = ! isFocusMode && ! hasFixedToolbar && isHovered && ! isEmptyDefaultBlock;
// We render block movers and block settings to keep them tabbale even if hidden
const shouldRenderMovers = ! isFocusMode && ( isSelected || hoverArea === 'left' ) && ! showEmptyBlockSideInserter && ! isMultiSelecting && ! isPartOfMultiSelection && ! isTypingWithinBlock;
Expand All @@ -410,7 +414,6 @@ export class BlockListBlock extends Component {
'has-warning': ! isValid || !! error || isUnregisteredBlock,
'is-selected': shouldAppearSelected,
'is-multi-selected': isPartOfMultiSelection,
'is-selected-parent': shouldAppearSelectedParent,
'is-hovered': shouldAppearHovered,
'is-reusable': isReusableBlock( blockType ),
'is-dragging': dragging,
Expand Down Expand Up @@ -592,7 +595,6 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV
getEditorSettings,
hasSelectedInnerBlock,
getTemplateLock,
hasMultiSelection,
} = select( 'core/editor' );
const isSelected = isBlockSelected( clientId );
const { hasFixedToolbar, focusMode } = getEditorSettings();
Expand All @@ -607,7 +609,6 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV
isPartOfMultiSelection: isBlockMultiSelected( clientId ) || isAncestorMultiSelected( clientId ),
isFirstMultiSelected: isFirstMultiSelectedBlock( clientId ),
isMultiSelecting: isMultiSelecting(),
hasSelectedInnerBlock: hasSelectedInnerBlock( clientId, false ),
// We only care about this prop when the block is selected
// Thus to avoid unnecessary rerenders we avoid updating the prop if the block is not selected.
isTypingWithinBlock: ( isSelected || isParentOfSelectedBlock ) && isTyping(),
Expand All @@ -626,7 +627,6 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV
block,
isSelected,
isParentOfSelectedBlock,
hasMultiSelection: hasMultiSelection(),
};
} );

Expand Down
1 change: 0 additions & 1 deletion packages/editor/src/components/block-list/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,6 @@
}

// Selected style
&.is-selected-parent > .editor-block-list__block-edit::before,
&.is-selected > .editor-block-list__block-edit::before {
// Use opacity to work in various editor styles.
outline: $border-width solid $dark-opacity-light-500;
Expand Down
46 changes: 46 additions & 0 deletions packages/editor/src/components/block-navigation/dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* WordPress dependencies
*/
import { Fragment } from '@wordpress/element';
import { IconButton, Dropdown, SVG, Path, KeyboardShortcuts } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { rawShortcut } from '@wordpress/keycodes';

/**
* Internal dependencies
*/
import BlockNavigation from './';

const menuIcon = (
<SVG xmlns="http://www.w3.org/2000/svg" viewBox="0 0 24 24" width="20">
<Path d="M5 5H3v2h2V5zm3 8h11v-2H8v2zm9-8H6v2h11V5zM7 11H5v2h2v-2zm0 8h2v-2H7v2zm3-2v2h11v-2H10z" />
</SVG>
);

function BlockNavigationDropdown() {
return (
<Dropdown
renderToggle={ ( { isOpen, onToggle } ) => (
<Fragment>
<KeyboardShortcuts
bindGlobal
shortcuts={ {
[ rawShortcut.access( 'b' ) ]: onToggle,
} }
/>
<IconButton
icon={ menuIcon }
aria-expanded={ isOpen }
onClick={ onToggle }
label={ __( 'Block Navigation' ) }
/>
</Fragment>
) }
renderContent={ ( { onClose } ) => (
<BlockNavigation onSelect={ onClose } />
) }
/>
);
}

export default BlockNavigationDropdown;
Loading