Adds the block hierarchy navigation menu to the header #10545
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -91,7 +91,7 @@ export class BlockListBlock extends Component { | |
| } | ||
|
|
||
| if ( this.props.isSelected && ! prevProps.isSelected ) { | ||
| this.focusTabbable( true ); | ||
|
There was a problem hiding this comment. What is it we're trying to do here? And why is it inconsistent with the behavior in There was a problem hiding this comment. 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? There was a problem hiding this comment. Maybe we should reconsider whether to call There was a problem hiding this comment.
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. There was a problem hiding this comment. As "Columns" evolve, it'd be natural to focus on the container when it is added rather than on the paragraph within. There was a problem hiding this comment.
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. There was a problem hiding this comment.
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. There was a problem hiding this comment. 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 There was a problem hiding this comment. 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". |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -118,8 +118,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( ignoreInnerBlocks ) { | ||
| const { initialPosition } = this.props; | ||
|
|
||
| // Focus is captured by the wrapper node, so while focus transition | ||
|
|
@@ -131,7 +133,12 @@ export class BlockListBlock extends Component { | |
| } | ||
|
|
||
| // Find all tabbables within node. | ||
| const innerBlocks = this.node.querySelector( '.editor-inner-blocks' ); | ||
|
There was a problem hiding this comment. This is a future bug waiting to happen. There was a problem hiding this comment. How would you solve it? There was a problem hiding this comment. I worked my way back to questioning whether we need it at all in following comments. We can revisit if still then necessary. There was a problem hiding this comment. We have an There was a problem hiding this comment. @aduth It won't work for the column's block appender. As it's in the same block but we don't want to focus it. There was a problem hiding this comment. Seems like a problem there is that we consider the default appender as a valid text field to target? For what reason is it There was a problem hiding this comment. I'd be content if we at least abstract this to some common utility in e.g. There was a problem hiding this comment. The appender is an input to allow the writing flow navigation (we discussed this at some point :) I think). I extracted to a dom utility and I've reused a className that was already used there to avoid creating a new point of failure. |
||
| const textInputs = focus.tabbable | ||
| .find( this.node ) | ||
| .filter( isTextField ) | ||
| // Exclude inner blocks | ||
| .filter( ( node ) => ! ignoreInnerBlocks || ! innerBlocks || ! innerBlocks.contains( node ) ); | ||
|
|
||
| // If reversed (e.g. merge via backspace), use the last in the set of | ||
| // tabbables. | ||
|
|
@@ -370,9 +377,7 @@ export class BlockListBlock extends Component { | |
| isEmptyDefaultBlock, | ||
| isMovable, | ||
| isPreviousBlockADefaultEmptyBlock, | ||
| isParentOfSelectedBlock, | ||
| isDraggable, | ||
| } = this.props; | ||
| const isHovered = this.state.isHovered && ! isMultiSelecting; | ||
|
|
@@ -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 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; | ||
|
|
@@ -410,7 +414,6 @@ export class BlockListBlock extends Component { | |
| 'has-warning': ! isValid || !! error || isUnregisteredBlock, | ||
| 'is-selected': shouldAppearSelected, | ||
| 'is-multi-selected': isPartOfMultiSelection, | ||
| 'is-hovered': shouldAppearHovered, | ||
| 'is-reusable': isReusableBlock( blockType ), | ||
| 'is-dragging': dragging, | ||
|
|
@@ -592,7 +595,6 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV | |
| getEditorSettings, | ||
| hasSelectedInnerBlock, | ||
| getTemplateLock, | ||
| } = select( 'core/editor' ); | ||
| const isSelected = isBlockSelected( clientId ); | ||
| const { hasFixedToolbar, focusMode } = getEditorSettings(); | ||
|
|
@@ -607,7 +609,6 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV | |
| isPartOfMultiSelection: isBlockMultiSelected( clientId ) || isAncestorMultiSelected( clientId ), | ||
| isFirstMultiSelected: isFirstMultiSelectedBlock( clientId ), | ||
| isMultiSelecting: isMultiSelecting(), | ||
| // 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(), | ||
|
|
@@ -626,7 +627,6 @@ const applyWithSelect = withSelect( ( select, { clientId, rootClientId, isLargeV | |
| block, | ||
| isSelected, | ||
| isParentOfSelectedBlock, | ||
| }; | ||
| } ); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,36 @@ | ||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { IconButton, Dropdown, SVG } from '@wordpress/components'; | ||
| import { __ } from '@wordpress/i18n'; | ||
|
|
||
| /** | ||
| * 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" /> | ||
|
There was a problem hiding this comment. Can you use There was a problem hiding this comment.
I did the hard work: There was a problem hiding this comment. Thanks Andrew,
I'll open a separate PR, as there's still some work to do :P There was a problem hiding this comment. Should we also use the There was a problem hiding this comment. What about rect...? |
||
| </SVG> | ||
| ); | ||
|
|
||
| function BlockNavigationDropdown() { | ||
| return ( | ||
| <Dropdown | ||
| renderToggle={ ( { isOpen, onToggle } ) => ( | ||
| <IconButton | ||
| icon={ menuIcon } | ||
| aria-expanded={ isOpen } | ||
| onClick={ onToggle } | ||
| label={ __( 'Block Navigation' ) } | ||
| /> | ||
| ) } | ||
| renderContent={ ( { onClose } ) => ( | ||
| <BlockNavigation onSelect={ onClose } /> | ||
| ) } | ||
| /> | ||
| ); | ||
| } | ||
|
|
||
| export default BlockNavigationDropdown; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,111 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import { map, noop } from 'lodash'; | ||
| import classnames from 'classnames'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { withSelect, withDispatch } from '@wordpress/data'; | ||
| import { MenuItem, MenuGroup } from '@wordpress/components'; | ||
| import { getBlockType } from '@wordpress/blocks'; | ||
| import { compose } from '@wordpress/compose'; | ||
|
|
||
| /** | ||
| * Internal dependencies | ||
| */ | ||
| import BlockIcon from '../block-icon'; | ||
|
|
||
| function BlockNavigationList( { | ||
| blocks, | ||
| selectedBlockClientId, | ||
| selectBlock, | ||
| showNestedBlocks, | ||
| } ) { | ||
| return ( | ||
| <ul className="editor-block-navigation__list"> | ||
| { map( blocks, ( block ) => { | ||
tofumatt marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| const blockType = getBlockType( block.name ); | ||
| return ( | ||
| <li key={ block.clientId }> | ||
| <div role="presentation" className="editor-block-navigation__item"> | ||
| <MenuItem | ||
| className={ classnames( 'editor-block-navigation__item-button', { | ||
| 'is-selected': block.clientId === selectedBlockClientId, | ||
| } ) } | ||
| onClick={ () => selectBlock( block.clientId ) } | ||
| isSelected={ block.clientId === selectedBlockClientId } | ||
| > | ||
| <BlockIcon icon={ blockType.icon } showColors /> | ||
| { blockType.title } | ||
| </MenuItem> | ||
| </div> | ||
| { showNestedBlocks && !! block.innerBlocks && !! block.innerBlocks.length && ( | ||
| <BlockNavigationList | ||
| blocks={ block.innerBlocks } | ||
| selectedBlockClientId={ selectedBlockClientId } | ||
| selectBlock={ selectBlock } | ||
| showNestedBlocks | ||
| /> | ||
| ) } | ||
| </li> | ||
| ); | ||
| } ) } | ||
| </ul> | ||
| ); | ||
| } | ||
|
|
||
| function BlockNavigation( { rootBlock, rootBlocks, selectedBlockClientId, selectBlock } ) { | ||
| const hasHierarchy = ( | ||
| rootBlock && ( | ||
| rootBlock.clientId !== selectedBlockClientId || | ||
| ( rootBlock.innerBlocks && rootBlock.innerBlocks.length !== 0 ) | ||
| ) | ||
| ); | ||
|
|
||
| return ( | ||
| <MenuGroup> | ||
| { hasHierarchy && ( | ||
| <BlockNavigationList | ||
| blocks={ [ rootBlock ] } | ||
| selectedBlockClientId={ selectedBlockClientId } | ||
| selectBlock={ selectBlock } | ||
| showNestedBlocks | ||
| /> | ||
| ) } | ||
| { ! hasHierarchy && ( | ||
| <BlockNavigationList | ||
| blocks={ rootBlocks } | ||
| selectedBlockClientId={ selectedBlockClientId } | ||
| selectBlock={ selectBlock } | ||
| /> | ||
| ) } | ||
| </MenuGroup> | ||
| ); | ||
| } | ||
|
|
||
| export default compose( | ||
| withSelect( ( select ) => { | ||
| const { | ||
| getSelectedBlockClientId, | ||
| getBlockHierarchyRootClientId, | ||
| getBlock, | ||
| getBlocks, | ||
| } = select( 'core/editor' ); | ||
| const selectedBlockClientId = getSelectedBlockClientId(); | ||
| return { | ||
| rootBlocks: getBlocks(), | ||
| rootBlock: selectedBlockClientId ? getBlock( getBlockHierarchyRootClientId( selectedBlockClientId ) ) : null, | ||
| selectedBlockClientId, | ||
| }; | ||
| } ), | ||
| withDispatch( ( dispatch, { onSelect = noop } ) => { | ||
| return { | ||
| selectBlock( clientId ) { | ||
| dispatch( 'core/editor' ).selectBlock( clientId ); | ||
| onSelect( clientId ); | ||
| }, | ||
| }; | ||
| } ) | ||
| )( BlockNavigation ); | ||
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.
Nitpick but (arguably) we don't need to wrap prose text in markdown files. It makes them harder to diff/edit/change over multiple lines and the alignment probably matters less, no? 🤷♂️