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

Show block icon in contentOnly toolbar #64694

Merged
merged 6 commits into from
Aug 29, 2024
Merged

Show block icon in contentOnly toolbar #64694

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1d62017
Extract block icon to be a BlockIndicator component
jeryj Aug 27, 2024
25db139
Remove ToolbarGroup that only ever has one item in it
jeryj Aug 27, 2024
872b8af
Show block icon in contentOnly toolbar
jeryj Aug 21, 2024
5939e0e
Make block icon same color as other toolbar items
jeryj Aug 27, 2024
3d3b0cd
Remove unnecessary opacity property
jeryj Aug 28, 2024
b5a9c8c
Revert removal of ToolbarGroup
jeryj Aug 29, 2024
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
46 changes: 26 additions & 20 deletions packages/block-editor/src/components/block-switcher/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,21 @@ function BlockSwitcherDropdownMenuContents( {
);
}

const BlockIndicator = ( { icon, showTitle, blockTitle } ) => (
<>
<BlockIcon
className="block-editor-block-switcher__toggle"
icon={ icon }
showColors
/>
{ showTitle && blockTitle && (
<span className="block-editor-block-switcher__toggle-text">
{ blockTitle }
</span>
) }
</>
);
Comment on lines +184 to +197
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 made this into its own component so it could be shared between the two places it's used. I'm tempted to fully extract it as I know I've ran into issues where I wanted to get the block indicator for a block but didn't because it wasn't going to be too much work.


export const BlockSwitcher = ( { clientIds, disabled, isUsingBindings } ) => {
const {
canRemove,
Expand Down Expand Up @@ -247,6 +262,7 @@ export const BlockSwitcher = ( { clientIds, disabled, isUsingBindings } ) => {
: __( 'Multiple blocks selected' );

const hideDropdown = disabled || ( ! hasBlockStyles && ! canRemove );

if ( hideDropdown ) {
return (
<ToolbarGroup>
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 removed the <ToolbarGroup /> from this and the switcher, as it will only ever have one item so doesn't feel semantically like a group... Also, it's already within a group.

Copy link
Member

Choose a reason for hiding this comment

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

This makes the BlockSwitcher the only component in the block-editor-block-toolbar__block-controls toolbar the group without the inner group. Both BlockLockToolbar and BlockMover have group wrappers.

Should we evaluate their removal separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably. I thought about this too. My guess is that it was a styling issue and not a technical one originally. But you're right that it should be evaluated separately. I'll add the ToolbarGroup in and put in another PR that removes the ToolbarGroups from those.

Copy link
Member

Choose a reason for hiding this comment

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

Thank you, @jeryj!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it back in 👍🏻

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the ToolbarGroups. We'll see if it breaks anything :) #64909

Expand All @@ -255,14 +271,11 @@ export const BlockSwitcher = ( { clientIds, disabled, isUsingBindings } ) => {
className="block-editor-block-switcher__no-switcher-icon"
title={ blockSwitcherLabel }
icon={
<>
<BlockIcon icon={ icon } showColors />
{ ( isReusable || isTemplate ) && (
<span className="block-editor-block-switcher__toggle-text">
{ blockTitle }
</span>
) }
</>
<BlockIndicator
icon={ icon }
showTitle={ isReusable || isTemplate }
blockTitle={ blockTitle }
/>
}
/>
</ToolbarGroup>
Expand Down Expand Up @@ -292,18 +305,11 @@ export const BlockSwitcher = ( { clientIds, disabled, isUsingBindings } ) => {
className: 'block-editor-block-switcher__popover',
} }
icon={
<>
<BlockIcon
icon={ icon }
className="block-editor-block-switcher__toggle"
showColors
/>
{ ( isReusable || isTemplate ) && (
<span className="block-editor-block-switcher__toggle-text">
{ blockTitle }
</span>
) }
</>
<BlockIndicator
icon={ icon }
showTitle={ isReusable || isTemplate }
blockTitle={ blockTitle }
/>
}
toggleProps={ {
description: blockSwitcherDescription,
Expand Down
7 changes: 3 additions & 4 deletions packages/block-editor/src/components/block-switcher/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,12 +46,11 @@
}

// Even when the block switcher does not have any transformations, it still serves as a block indicator.
.components-button.block-editor-block-switcher__no-switcher-icon:disabled {
opacity: 1;
.components-button.block-editor-block-switcher__no-switcher-icon[aria-disabled="true"] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It wasn't applying the CSS until switching to aria-disabled.

color: $gray-900;

// Since it's not clickable, though, don't show a hover state.
&,
.block-editor-block-icon.has-colors {
&:hover {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Matching the comment worked to keep this consistent.

color: $gray-900;
}
}
Expand Down
10 changes: 7 additions & 3 deletions packages/block-editor/src/components/block-toolbar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export function PrivateBlockToolbar( {
const {
blockClientId,
blockClientIds,
isContentOnlyEditingMode,
isDefaultEditingMode,
blockType,
toolbarKey,
Expand All @@ -83,8 +84,8 @@ export function PrivateBlockToolbar( {
const firstParentClientId = parents[ parents.length - 1 ];
const parentBlockName = getBlockName( firstParentClientId );
const parentBlockType = getBlockType( parentBlockName );
const _isDefaultEditingMode =
getBlockEditingMode( selectedBlockClientId ) === 'default';
const editingMode = getBlockEditingMode( selectedBlockClientId );
const _isDefaultEditingMode = editingMode === 'default';
const _blockName = getBlockName( selectedBlockClientId );
const isValid = selectedBlockClientIds.every( ( id ) =>
isBlockValid( id )
Expand All @@ -99,6 +100,7 @@ export function PrivateBlockToolbar( {
return {
blockClientId: selectedBlockClientId,
blockClientIds: selectedBlockClientIds,
isContentOnlyEditingMode: editingMode === 'contentOnly',
isDefaultEditingMode: _isDefaultEditingMode,
blockType: selectedBlockClientId && getBlockType( _blockName ),
shouldShowVisualToolbar: isValid && isVisual,
Expand Down Expand Up @@ -168,7 +170,9 @@ export function PrivateBlockToolbar( {
isLargeViewport &&
isDefaultEditingMode && <BlockParentSelector /> }
{ ( shouldShowVisualToolbar || isMultiToolbar ) &&
( isDefaultEditingMode || isSynced ) && (
( isDefaultEditingMode ||
isContentOnlyEditingMode ||
isSynced ) && (
<div
ref={ nodeRef }
{ ...showHoveredOrFocusedGestures }
Expand Down
Loading