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

Jump to bottom

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

Merged
merged 4 commits into from
Jun 12, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
744a165
Fix showing double icons for connected blocks in pattern editor
kevin940726 Jun 5, 2024
d7cc674
Fix post-meta
kevin940726 Jun 5, 2024
c862489
Unify the experience
kevin940726 Jun 7, 2024
72d0a8e
Rename the component
kevin940726 Jun 11, 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
12 changes: 0 additions & 12 deletions packages/block-editor/src/components/block-bindings-toolbar-indicator/style.scss
View file
Open in desktop

This file was deleted.

26 changes: 24 additions & 2 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
@@ -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
5 changes: 5 additions & 0 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 @@ -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
View file
Open in desktop
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
View file
Open in desktop
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
61 changes: 22 additions & 39 deletions ...block-bindings-toolbar-indicator/index.js → ...tern-overrides-toolbar-indicator/index.js
View file
Open in desktop
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
12 changes: 12 additions & 0 deletions packages/block-editor/src/components/pattern-overrides-toolbar-indicator/style.scss
View file
Open in desktop
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
View file
Open in desktop
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