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

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 3 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
47 changes: 12 additions & 35 deletions packages/block-editor/src/components/block-bindings-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 @@ -22,13 +22,10 @@ import useBlockDisplayTitle from '../block-title/use-block-display-title';

export default function BlockBindingsToolbarIndicator( { clientIds } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I am not mistaken, this component becomes specific to pattern overrides because it is only used when adding a pattern in a post, right? Maybe we can rename it and slightly change the implementation to reflect that?

Copy link
Member Author

Choose a reason for hiding this comment

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

On second thought, I don't think this even belongs in the block-editor package 😅. I'd imagine it'll be a bigger refactor if we figure out a way to move it to the patterns package though. I'd prefer to do it in a follow-up PR if necessary. WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

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

Moving it to the patterns package seems logical to me 🙂 And I guess it is okay to do it in a follow-up PR. Should we create an issue or something to ensure it is not lost?

Maybe it is still worth changing the component name to clarify it is related to patterns and not block bindings until that PR is done?

Copy link
Member Author

@kevin940726 kevin940726 Jun 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Done in 72d0a8e! I renamed some stuff, so could appreciate a final review to ensure I didn't break anything 😆.

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 +51,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,25 +60,15 @@ 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 (
Expand Down
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;
}
24 changes: 18 additions & 6 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 @@ -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,11 @@ export function PrivateBlockToolbar( {
{ ! isMultiToolbar &&
isLargeViewport &&
isDefaultEditingMode && <BlockParentSelector /> }
{ isUsingBindings && canBindBlock( blockName ) && (
<BlockBindingsIndicator clientIds={ blockClientIds } />
) }
{ isUsingBindings &&
hasParentPattern &&
canBindBlock( blockName ) && (
<BlockBindingsIndicator clientIds={ blockClientIds } />
) }
{ ( shouldShowVisualToolbar || isMultiToolbar ) &&
( isDefaultEditingMode || isSynced ) && (
<div
Expand All @@ -180,6 +191,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
Loading