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

Refactor: useBlockTools hook #58979

Merged
merged 12 commits into from
Feb 28, 2024
14 changes: 3 additions & 11 deletions packages/block-editor/src/components/block-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,8 @@ import { useShowHoveredOrFocusedGestures } from './utils';
import { store as blockEditorStore } from '../../store';
import __unstableBlockNameContext from './block-name-context';
import NavigableToolbar from '../navigable-toolbar';
import { useHasAnyBlockControls } from '../block-controls/use-has-block-controls';
import Shuffle from './shuffle';

import { useHasBlockToolbar } from './use-has-block-toolbar';
/**
* Renders the block toolbar.
*
Expand Down Expand Up @@ -122,15 +121,8 @@ export function PrivateBlockToolbar( {

const isLargeViewport = ! useViewportMatch( 'medium', '<' );

const isToolbarEnabled =
blockType &&
hasBlockSupport( blockType, '__experimentalToolbar', true );
const hasAnyBlockControls = useHasAnyBlockControls();

if (
! isToolbarEnabled ||
( ! isDefaultEditingMode && ! hasAnyBlockControls )
) {
const hasBlockToolbar = useHasBlockToolbar();
if ( ! hasBlockToolbar ) {
return null;
}

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { getBlockType, hasBlockSupport } from '@wordpress/blocks';
/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../../store';
import { useHasAnyBlockControls } from '../block-controls/use-has-block-controls';

/**
* Returns true if the block toolbar should be shown.
*
* @return {boolean} Whether the block toolbar component will be rendered.
*/
export function useHasBlockToolbar() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export function useHasBlockToolbar() {
export function useShowBlockToolbar() {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't a check to know if it will show or not, but rather if it can be shown. Like, does the current selection have a block toolbar it can render? useShowBlockToolbar makes me think it will do something to display the toolbar.

const hasAnyBlockControls = useHasAnyBlockControls();
return useSelect(
( select ) => {
const {
getBlockEditingMode,
getBlockName,
getSelectedBlockClientIds,
} = select( blockEditorStore );

const selectedBlockClientIds = getSelectedBlockClientIds();
const selectedBlockClientId = selectedBlockClientIds[ 0 ];
Comment on lines +27 to +28
Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: We can probably use getBlockSelectionStart here if we want only the first block clientId.

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 do think that would be a good change because getBlockSelectionStart() returns a state value and selectedBlockClientIds() does quite a bit more work. However, if I:

  • Add three paragraphs
  • click and drag from the third paragraph up to the first
  • getBlockSelectionStart() will return the third paragraph ID (where selection started) and getSelectedBlockClientIds()[0] will return the first paragraph ID.

So, as it would be a functional change, I think we should do it in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Follow-up at #59450

const isDefaultEditingMode =
getBlockEditingMode( selectedBlockClientId ) === 'default';
const blockType =
selectedBlockClientId &&
getBlockType( getBlockName( selectedBlockClientId ) );
const isToolbarEnabled =
blockType &&
hasBlockSupport( blockType, '__experimentalToolbar', true );

if (
! isToolbarEnabled ||
( ! isDefaultEditingMode && ! hasAnyBlockControls )
) {
return false;
}
Comment on lines +38 to +43
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is the same check that block-toolbar.js used to determine if it would early return or continue to render.


return true;
},
[ hasAnyBlockControls ]
);
}
49 changes: 13 additions & 36 deletions packages/block-editor/src/components/block-tools/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@ import { useSelect, useDispatch } from '@wordpress/data';
import { Popover } from '@wordpress/components';
import { __unstableUseShortcutEventMatch as useShortcutEventMatch } from '@wordpress/keyboard-shortcuts';
import { useRef } from '@wordpress/element';
import { isUnmodifiedDefaultBlock } from '@wordpress/blocks';

/**
* Internal dependencies
Expand All @@ -20,51 +19,27 @@ import BlockToolbarBreadcrumb from './block-toolbar-breadcrumb';
import { store as blockEditorStore } from '../../store';
import usePopoverScroll from '../block-popover/use-popover-scroll';
import ZoomOutModeInserters from './zoom-out-mode-inserters';
import { useShowBlockTools } from './use-show-block-tools';

function selector( select ) {
const {
getSelectedBlockClientId,
getFirstMultiSelectedBlockClientId,
getBlock,
getSettings,
hasMultiSelection,
__unstableGetEditorMode,
isTyping,
} = select( blockEditorStore );

const clientId =
getSelectedBlockClientId() || getFirstMultiSelectedBlockClientId();

const { name = '', attributes = {} } = getBlock( clientId ) || {};
const editorMode = __unstableGetEditorMode();
const hasSelectedBlock = clientId && name;
const isEmptyDefaultBlock = isUnmodifiedDefaultBlock( {
name,
attributes,
} );
const _showEmptyBlockSideInserter =
clientId &&
! isTyping() &&
editorMode === 'edit' &&
isUnmodifiedDefaultBlock( { name, attributes } );
const maybeShowBreadcrumb =
hasSelectedBlock &&
! hasMultiSelection() &&
( editorMode === 'navigation' || editorMode === 'zoom-out' );

return {
clientId,
hasFixedToolbar: getSettings().hasFixedToolbar,
isTyping: isTyping(),
isZoomOutMode: editorMode === 'zoom-out',
showEmptyBlockSideInserter: _showEmptyBlockSideInserter,
showBreadcrumb: ! _showEmptyBlockSideInserter && maybeShowBreadcrumb,
showBlockToolbar:
! getSettings().hasFixedToolbar &&
! _showEmptyBlockSideInserter &&
hasSelectedBlock &&
! isEmptyDefaultBlock &&
! maybeShowBreadcrumb,
};
}

Expand All @@ -82,18 +57,20 @@ export default function BlockTools( {
__unstableContentRef,
...props
} ) {
const {
clientId,
hasFixedToolbar,
isTyping,
isZoomOutMode,
showEmptyBlockSideInserter,
showBreadcrumb,
showBlockToolbar,
} = useSelect( selector, [] );
const { clientId, hasFixedToolbar, isTyping, isZoomOutMode } = useSelect(
selector,
[]
);
const isMatch = useShortcutEventMatch();
const { getSelectedBlockClientIds, getBlockRootClientId } =
useSelect( blockEditorStore );

const {
showEmptyBlockSideInserter,
showBreadcrumb,
showBlockToolbarPopover,
} = useShowBlockTools();

const {
duplicateBlocks,
removeBlocks,
Expand Down Expand Up @@ -186,7 +163,7 @@ export default function BlockTools( {
/>
) }

{ showBlockToolbar && (
{ showBlockToolbarPopover && (
<BlockToolbarPopover
__unstableContentRef={ __unstableContentRef }
clientId={ clientId }
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
/**
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { isUnmodifiedDefaultBlock } from '@wordpress/blocks';

/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../../store';
import { useHasBlockToolbar } from '../block-toolbar/use-has-block-toolbar';

/**
* Source of truth for which block tools are showing in the block editor.
*
* @return {Object} Object of which block tools will be shown.
*/
export function useShowBlockTools() {
const hasBlockToolbar = useHasBlockToolbar();

return useSelect(
( select ) => {
const {
getSelectedBlockClientId,
getFirstMultiSelectedBlockClientId,
getBlock,
getSettings,
hasMultiSelection,
__unstableGetEditorMode,
isTyping,
} = select( blockEditorStore );

const clientId =
getSelectedBlockClientId() ||
getFirstMultiSelectedBlockClientId();

const { name = '', attributes = {} } = getBlock( clientId ) || {};
const editorMode = __unstableGetEditorMode();
const hasSelectedBlock = clientId && name;
const isEmptyDefaultBlock = isUnmodifiedDefaultBlock( {
name,
attributes,
} );
const _showEmptyBlockSideInserter =
clientId &&
! isTyping() &&
editorMode === 'edit' &&
isUnmodifiedDefaultBlock( { name, attributes } );
const maybeShowBreadcrumb =
hasSelectedBlock &&
! hasMultiSelection() &&
( editorMode === 'navigation' || editorMode === 'zoom-out' );

return {
showEmptyBlockSideInserter: _showEmptyBlockSideInserter,
showBreadcrumb:
! _showEmptyBlockSideInserter && maybeShowBreadcrumb,
showBlockToolbarPopover:
hasBlockToolbar &&
! getSettings().hasFixedToolbar &&
! _showEmptyBlockSideInserter &&
hasSelectedBlock &&
! isEmptyDefaultBlock &&
! maybeShowBreadcrumb,
showFixedToolbar:
editorMode !== 'zoom-out' &&
hasBlockToolbar &&
getSettings().hasFixedToolbar,
Comment on lines +55 to +68
Copy link
Member

Choose a reason for hiding this comment

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

This hook subscribes to four values. Three of these are only used by BlockTools and the remaining showFixedToolbar by only editor packages.

I'm unsure if moving the selection of values used only by BlockTools to this hook is worth it. cc @ellatrix

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 had concerns about that too. However, the information we need for showFixedToolbar is all right here, and this hook is already running for <BlockTools />. It doesn't need to do any additional work to set this value. I think it will end up being more performant and less code, so it's worth the tradeoff.

Copy link
Member

Choose a reason for hiding this comment

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

This is probably fine. We can always refactor if CodeVitals catches any perf regression.

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 fine, it's not worth trying to optimise performance here because there's only one block toolbar visible at any point in time.

Copy link
Member

Choose a reason for hiding this comment

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

Or maybe not, looking at the comments below. Not sure if this is the cause of the regression though.

};
},
[ hasBlockToolbar ]
);
}
4 changes: 2 additions & 2 deletions packages/block-editor/src/private-apis.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { ComposedPrivateInserter as PrivateInserter } from './components/inserte
import { default as PrivateQuickInserter } from './components/inserter/quick-inserter';
import { PrivateListView } from './components/list-view';
import BlockInfo from './components/block-info-slot-fill';
import { useCanBlockToolbarBeFocused } from './utils/use-can-block-toolbar-be-focused';
import { useShowBlockTools } from './components/block-tools/use-show-block-tools';
import { cleanEmptyObject, useStyleOverride } from './hooks/utils';
import BlockQuickNavigation from './components/block-quick-navigation';
import { LayoutStyle } from './components/block-list/layout';
Expand Down Expand Up @@ -44,7 +44,7 @@ lock( privateApis, {
PrivateListView,
ResizableBoxPopover,
BlockInfo,
useCanBlockToolbarBeFocused,
useShowBlockTools,
cleanEmptyObject,
useStyleOverride,
BlockQuickNavigation,
Expand Down

This file was deleted.

Loading
Loading