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

Fix showing double icons for connected blocks in pattern editor #62317

Merged
merged 4 commits into from
Jun 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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

This file was deleted.

26 changes: 24 additions & 2 deletions packages/block-editor/src/components/block-switcher/index.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,14 @@
/**
* WordPress dependencies
*/
import { __, _n, sprintf } from '@wordpress/i18n';
import { __, _n, sprintf, _x } from '@wordpress/i18n';
import {
DropdownMenu,
ToolbarButton,
ToolbarGroup,
ToolbarItem,
__experimentalText as Text,
MenuGroup,
} from '@wordpress/components';
import {
switchToBlockType,
Expand All @@ -33,6 +35,7 @@ function BlockSwitcherDropdownMenuContents( {
clientIds,
hasBlockStyles,
canRemove,
isUsingBindings,
} ) {
const { replaceBlocks, multiSelect, updateBlockAttributes } =
useDispatch( blockEditorStore );
Expand Down Expand Up @@ -118,6 +121,17 @@ function BlockSwitcherDropdownMenuContents( {
</p>
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we could get isUsingBindings from this useSelect. Something similar to what is being done here.

If we do this, I believe we could avoid passing isUsingBindings to BlockSwitcher and BlockSwitcherDropdownMenuContents components.

Additionally, I think we wouldn't need isUsingBindings in the block toolbar anymore (here) and we could just have something like hasPatternOverrides, as it is now specific to pattern overrides, right?

I am not 100% sure if it makes sense, but maybe it is worth taking a look.

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe so, yes! However I don't think it makes much difference. Currently we still need isUsingBindings in block toolbar to apply the is-connected class name. We can somehow move the class name to block switcher, but in terms of performance I don't think it matters that much. Do you think it's worth exploring?

Copy link
Contributor

Choose a reason for hiding this comment

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

I wasn't thinking about performance but about readability, but I don't have a strong opinion, so whatever you decide it's okay for me 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it reads okay for me, but I'm obviously biased 😅. Happy to do some further improvements if we find something confusing later! 🙇


const connectedBlockDescription = isSingleBlock
? _x(
'This block is connected.',
'block toolbar button label and description'
)
: _x(
'These blocks are connected.',
'block toolbar button label and description'
);

return (
<div className="block-editor-block-switcher__container">
{ hasPatternTransformation && (
Expand Down Expand Up @@ -156,11 +170,18 @@ function BlockSwitcherDropdownMenuContents( {
onSwitch={ onClose }
/>
) }
{ isUsingBindings && (
<MenuGroup>
<Text className="block-editor-block-switcher__binding-indicator">
{ connectedBlockDescription }
</Text>
</MenuGroup>
) }
</div>
);
}

export const BlockSwitcher = ( { clientIds, disabled } ) => {
export const BlockSwitcher = ( { clientIds, disabled, isUsingBindings } ) => {
const {
canRemove,
hasBlockStyles,
Expand Down Expand Up @@ -303,6 +324,7 @@ export const BlockSwitcher = ( { clientIds, disabled } ) => {
clientIds={ clientIds }
hasBlockStyles={ hasBlockStyles }
canRemove={ canRemove }
isUsingBindings={ isUsingBindings }
/>
) }
</DropdownMenu>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,3 +203,8 @@
padding: 6px $grid-unit;
margin: 0;
}

.block-editor-block-switcher__binding-indicator {
display: block;
padding: $grid-unit;
}
28 changes: 21 additions & 7 deletions packages/block-editor/src/components/block-toolbar/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ import { store as blockEditorStore } from '../../store';
import __unstableBlockNameContext from './block-name-context';
import NavigableToolbar from '../navigable-toolbar';
import Shuffle from './shuffle';
import BlockBindingsIndicator from '../block-bindings-toolbar-indicator';
import PatternOverridesToolbarIndicator from '../pattern-overrides-toolbar-indicator';
import { useHasBlockToolbar } from './use-has-block-toolbar';
import { canBindBlock } from '../../hooks/use-bindings-attributes';
/**
Expand Down Expand Up @@ -67,11 +67,13 @@ export function PrivateBlockToolbar( {
shouldShowVisualToolbar,
showParentSelector,
isUsingBindings,
hasParentPattern,
} = useSelect( ( select ) => {
const {
getBlockName,
getBlockMode,
getBlockParents,
getBlockParentsByBlockName,
getSelectedBlockClientIds,
isBlockValid,
getBlockRootClientId,
Expand All @@ -94,8 +96,13 @@ export function PrivateBlockToolbar( {
const isVisual = selectedBlockClientIds.every(
( id ) => getBlockMode( id ) === 'visual'
);
const _isUsingBindings = !! getBlockAttributes( selectedBlockClientId )
?.metadata?.bindings;
const bindings = getBlockAttributes( selectedBlockClientId )?.metadata
?.bindings;
const parentPatternClientId = getBlockParentsByBlockName(
selectedBlockClientId,
'core/block',
true
)[ 0 ];
return {
blockClientId: selectedBlockClientId,
blockClientIds: selectedBlockClientIds,
Expand All @@ -115,7 +122,8 @@ export function PrivateBlockToolbar( {
) &&
selectedBlockClientIds.length === 1 &&
_isDefaultEditingMode,
isUsingBindings: _isUsingBindings,
isUsingBindings: !! bindings,
hasParentPattern: !! parentPatternClientId,
};
}, [] );

Expand Down Expand Up @@ -146,6 +154,7 @@ export function PrivateBlockToolbar( {

const innerClasses = clsx( 'block-editor-block-toolbar', {
'is-synced': isSynced,
'is-connected': isUsingBindings,
} );

return (
Expand All @@ -167,9 +176,13 @@ export function PrivateBlockToolbar( {
{ ! isMultiToolbar &&
isLargeViewport &&
isDefaultEditingMode && <BlockParentSelector /> }
{ isUsingBindings && canBindBlock( blockName ) && (
<BlockBindingsIndicator clientIds={ blockClientIds } />
) }
{ isUsingBindings &&
hasParentPattern &&
canBindBlock( blockName ) && (
<PatternOverridesToolbarIndicator
clientIds={ blockClientIds }
/>
) }
{ ( shouldShowVisualToolbar || isMultiToolbar ) &&
( isDefaultEditingMode || isSynced ) && (
<div
Expand All @@ -180,6 +193,7 @@ export function PrivateBlockToolbar( {
<BlockSwitcher
clientIds={ blockClientIds }
disabled={ ! isDefaultEditingMode }
isUsingBindings={ isUsingBindings }
/>
{ isDefaultEditingMode && (
<>
Expand Down
15 changes: 9 additions & 6 deletions packages/block-editor/src/components/block-toolbar/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,17 @@
border-right: $border-width solid $gray-300;
}

&.is-synced .block-editor-block-switcher .components-button .block-editor-block-icon {
color: var(--wp-block-synced-color);
}

&.is-synced .components-toolbar-button.block-editor-block-switcher__no-switcher-icon {
&:disabled .block-editor-block-icon.has-colors {
&.is-synced,
&.is-connected {
.block-editor-block-switcher .components-button .block-editor-block-icon {
color: var(--wp-block-synced-color);
}

.components-toolbar-button.block-editor-block-switcher__no-switcher-icon {
&:disabled .block-editor-block-icon.has-colors {
color: var(--wp-block-synced-color);
}
}
}

> :last-child,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* WordPress dependencies
*/
import { useId } from '@wordpress/element';
import { __, sprintf, _x } from '@wordpress/i18n';
import { __, sprintf } from '@wordpress/i18n';
import {
DropdownMenu,
ToolbarGroup,
Expand All @@ -20,15 +20,18 @@ import { store as blockEditorStore } from '../../store';
import BlockIcon from '../block-icon';
import useBlockDisplayTitle from '../block-title/use-block-display-title';

export default function BlockBindingsToolbarIndicator( { clientIds } ) {
/**
* This component is currently only for pattern overrides, which is a WP-only feature.
* Ideally, this should be moved to the `patterns` package once ready.
* @param {Object} props The component props.
* @param {Array} props.clientIds The client IDs of the selected blocks.
*/
export default function PatternOverridesToolbarIndicator( { clientIds } ) {
const isSingleBlockSelected = clientIds.length === 1;
const { icon, firstBlockName, isConnectedToPatternOverrides } = useSelect(
const { icon, firstBlockName } = useSelect(
( select ) => {
const {
getBlockAttributes,
getBlockNamesByClientId,
getBlocksByClientId,
} = select( blockEditorStore );
const { getBlockAttributes, getBlockNamesByClientId } =
select( blockEditorStore );
const { getBlockType, getActiveBlockVariation } =
select( blocksStore );
const blockTypeNames = getBlockNamesByClientId( clientIds );
Expand All @@ -54,16 +57,6 @@ export default function BlockBindingsToolbarIndicator( { clientIds } ) {
icon: _icon,
firstBlockName: getBlockAttributes( clientIds[ 0 ] ).metadata
.name,
isConnectedToPatternOverrides: getBlocksByClientId(
clientIds
).some( ( block ) =>
Object.values(
block?.attributes?.metadata?.bindings || {}
).some(
( binding ) =>
binding.source === 'core/pattern-overrides'
)
),
};
},
[ clientIds, isSingleBlockSelected ]
Expand All @@ -73,44 +66,34 @@ export default function BlockBindingsToolbarIndicator( { clientIds } ) {
maximumLength: 35,
} );

let blockDescription = isSingleBlockSelected
? _x(
'This block is connected.',
'block toolbar button label and description'
const blockDescription = isSingleBlockSelected
? sprintf(
/* translators: %1s: The block type's name; %2s: The block's user-provided name (the same as the override name). */
__( 'This %1$s is editable using the "%2$s" override.' ),
firstBlockTitle.toLowerCase(),
firstBlockName
)
: _x(
'These blocks are connected.',
'block toolbar button label and description'
);
if ( isConnectedToPatternOverrides && firstBlockName ) {
blockDescription = isSingleBlockSelected
? sprintf(
/* translators: %1s: The block type's name; %2s: The block's user-provided name (the same as the override name). */
__( 'This %1$s is editable using the "%2$s" override.' ),
firstBlockTitle.toLowerCase(),
firstBlockName
)
: __( 'These blocks are editable using overrides.' );
}
: __( 'These blocks are editable using overrides.' );

const descriptionId = useId();

return (
<ToolbarGroup>
<ToolbarItem>
{ ( toggleProps ) => (
<DropdownMenu
className="block-editor-block-bindings-toolbar-indicator"
className="block-editor-pattern-overrides-toolbar-indicator"
label={ firstBlockTitle }
popoverProps={ {
placement: 'bottom-start',
className:
'block-editor-block-bindings-toolbar-indicator__popover',
'block-editor-pattern-overrides-toolbar-indicator__popover',
} }
icon={
<>
<BlockIcon
icon={ icon }
className="block-editor-block-bindings-toolbar-indicator-icon"
className="block-editor-pattern-overrides-toolbar-indicator-icon"
showColors
/>
</>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
.block-editor-pattern-overrides-toolbar-indicator__popover .components-popover__content {
min-width: 260px;
padding: $grid-unit-20;
}

.block-editor-pattern-overrides-toolbar-indicator .block-editor-pattern-overrides-toolbar-indicator-icon.has-colors svg {
fill: var(--wp-block-synced-color);
}

.editor-collapsible-block-toolbar .block-editor-pattern-overrides-toolbar-indicator {
height: 32px;
}
2 changes: 1 addition & 1 deletion packages/block-editor/src/style.scss
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
@import "./autocompleters/style.scss";
@import "./components/block-alignment-control/style.scss";
@import "./components/block-bindings-toolbar-indicator/style.scss";
@import "./components/pattern-overrides-toolbar-indicator/style.scss";
@import "./components/block-canvas/style.scss";
@import "./components/block-icon/style.scss";
@import "./components/block-inspector/style.scss";
Expand Down
Loading