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

Block bindings: UI for connecting bindings #62880

Merged
merged 81 commits into from
Jul 30, 2024
Merged
Show file tree
Hide file tree
Changes from 76 commits
Commits
Show all changes
81 commits
Select commit Hold shift + click to select a range
ae6afbc
Initial experiment
cbravobernal Jul 25, 2024
5efa047
First experiment of raw custom field for P
cbravobernal Jul 25, 2024
0a1b636
use selectors
cbravobernal Jul 25, 2024
1f9900e
Use lists
cbravobernal Jul 25, 2024
8128c0f
Remove
cbravobernal Jul 25, 2024
4eac7dc
Fix removing
cbravobernal Jul 25, 2024
db906bd
Add new ui
cbravobernal Jul 25, 2024
e357882
Update attributes
cbravobernal Jul 25, 2024
1a18fb6
Check binded number
cbravobernal Jul 25, 2024
39b2efa
Check binded number
cbravobernal Jul 25, 2024
8484002
Move bindings panel to editor
cbravobernal Jul 25, 2024
88759d3
Move code
cbravobernal Jul 25, 2024
aa626fe
Move to utils
cbravobernal Jul 25, 2024
be20d24
Use tools panel for connections
cbravobernal Jul 25, 2024
5343678
Add reset
cbravobernal Jul 25, 2024
58054eb
Add bordered items
cbravobernal Jul 25, 2024
8e8a122
Fix some styles
cbravobernal Jul 25, 2024
683abfb
Move utils back to component
cbravobernal Jul 25, 2024
d3ba547
Remove not needed comments
cbravobernal Jul 25, 2024
3b703f7
Some refactor
cbravobernal Jul 25, 2024
f0cd95a
Prevent UI with no meta data
cbravobernal Jul 25, 2024
3bc6072
Add helper
cbravobernal Jul 25, 2024
da6845e
Add helper
cbravobernal Jul 25, 2024
28ceffe
Add simple e2e
cbravobernal Jul 25, 2024
a90d9da
Add one more menuitem to the list
cbravobernal Jul 25, 2024
2df3ef3
Update e2e tests now a new tab appears by default
cbravobernal Jul 25, 2024
6be1849
Another test
cbravobernal Jul 25, 2024
0380ee8
Add e2e to check paragraph
cbravobernal Jul 25, 2024
fe88757
Remove not needed experiment
cbravobernal Jul 25, 2024
41691df
Remove extra blank spaces
cbravobernal Jul 25, 2024
4f097d0
update e2e
cbravobernal Jul 25, 2024
6e04036
Keep updating e2e
cbravobernal Jul 25, 2024
51904df
Remove css to the minimum
cbravobernal Jul 25, 2024
28ba254
Less CSS
cbravobernal Jul 25, 2024
25051fc
Abstract fields and move hooks back to block-editor
SantosGuillamot Jul 25, 2024
981367a
Return null when no bindableAttributes
SantosGuillamot Jul 25, 2024
b1aaa7a
Add checks to registration
SantosGuillamot Jul 25, 2024
3d9182e
Fix when the list of fields is empty
SantosGuillamot Jul 25, 2024
ff7c393
Properly filter footnotes
SantosGuillamot Jul 25, 2024
f2469c9
Update tests
cbravobernal Jul 25, 2024
26b5988
Remove classes from component
cbravobernal Jul 25, 2024
8798953
Remove all components mention
cbravobernal Jul 25, 2024
27639ba
Add ignore console error
cbravobernal Jul 25, 2024
f0a0680
Add icons and fix styling
cbravobernal Jul 25, 2024
42693a4
Remove not needed as p
cbravobernal Jul 25, 2024
b7efec2
Remove icon
cbravobernal Jul 25, 2024
11b2a4b
Remove icons, show label only if there are more than one source
cbravobernal Jul 25, 2024
58eb28c
Simplify style names
cbravobernal Jul 25, 2024
d17b593
Adapt design
cbravobernal Jul 25, 2024
dfeba8b
Use `ItemGroup` inside `ToolsPanel`
SantosGuillamot Jul 25, 2024
9e67785
Use `Text` component
SantosGuillamot Jul 25, 2024
b19d01e
Use `getBlockBindingsSources` function
SantosGuillamot Jul 25, 2024
fb4a51b
Revert change to popover max-width
SantosGuillamot Jul 25, 2024
6439308
Remove not used classes
SantosGuillamot Jul 25, 2024
9f277b3
Remove unused props
SantosGuillamot Jul 25, 2024
0beff34
Add fallbacks for undefined keys
SantosGuillamot Jul 25, 2024
8e6ad14
Try: Use `DropdownMenuV2`
SantosGuillamot Jul 25, 2024
c9ea7c7
Add button to remove the binding
SantosGuillamot Jul 25, 2024
436c97f
Reduce truncate lines to 1
SantosGuillamot Jul 25, 2024
9896dbf
Fix labels when multiple sources
SantosGuillamot Jul 25, 2024
752d7e0
Try: Add readOnly support when fields are undefined
SantosGuillamot Jul 25, 2024
feb890d
Use RadioItem instead of Checkbox
SantosGuillamot Jul 25, 2024
8bb9136
Remove "Clear" button
SantosGuillamot Jul 25, 2024
22e1020
Remove clear button CSS
SantosGuillamot Jul 25, 2024
cabd3d8
Don't use DropdownItems outside Dropdown
SantosGuillamot Jul 25, 2024
b814725
Move separator inside menu group
SantosGuillamot Jul 25, 2024
591bd72
Use onChange to better align with radio behavior
artemiomorales Jul 25, 2024
8ffc7f4
Add aria-hiddden to group heading
artemiomorales Jul 25, 2024
13cd09e
Update tests
artemiomorales Jul 25, 2024
3a086ab
Reduce custom styles
SantosGuillamot Jul 25, 2024
12c9088
Properly add block context after latest changes
SantosGuillamot Jul 25, 2024
bc05788
Adapt tests
SantosGuillamot Jul 25, 2024
a9b347f
Remove `numberOfLines` prop
SantosGuillamot Jul 25, 2024
839b37a
Remove unused class
SantosGuillamot Jul 25, 2024
01fcf7d
Fix mobile popover
SantosGuillamot Jul 25, 2024
d14d50f
Move Separator outside of `DropdownMenuGroup`
SantosGuillamot Jul 25, 2024
2dff252
Remove empty sources from `fieldsList`
SantosGuillamot Jul 29, 2024
50f6f1e
Move UI under experimental flag
SantosGuillamot Jul 30, 2024
3bf117c
Adapt e2e changes to experimental flag
SantosGuillamot Jul 30, 2024
6fb7d89
Remove missing clicks in tests
SantosGuillamot Jul 30, 2024
d091c34
Adapt another test
SantosGuillamot Jul 30, 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
347 changes: 295 additions & 52 deletions packages/block-editor/src/hooks/block-bindings.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,42 +2,199 @@
* WordPress dependencies
*/
import { __ } from '@wordpress/i18n';
import { store as blocksStore } from '@wordpress/blocks';
import { privateApis as blocksPrivateApis } from '@wordpress/blocks';
import {
BaseControl,
PanelBody,
__experimentalHStack as HStack,
__experimentalItemGroup as ItemGroup,
__experimentalItem as Item,
__experimentalText as Text,
__experimentalToolsPanel as ToolsPanel,
__experimentalToolsPanelItem as ToolsPanelItem,
__experimentalTruncate as Truncate,
__experimentalVStack as VStack,
privateApis as componentsPrivateApis,
} from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { useSelect, useDispatch, useRegistry } from '@wordpress/data';
import { useContext, Fragment } from '@wordpress/element';
import { useViewportMatch } from '@wordpress/compose';

/**
* Internal dependencies
*/
import { canBindAttribute } from '../hooks/use-bindings-attributes';
import {
canBindAttribute,
getBindableAttributes,
} from '../hooks/use-bindings-attributes';
import { store as blockEditorStore } from '../store';
import { unlock } from '../lock-unlock';
import InspectorControls from '../components/inspector-controls';
import BlockContext from '../components/block-context';

const {
DropdownMenuV2: DropdownMenu,
DropdownMenuGroupV2: DropdownMenuGroup,
DropdownMenuRadioItemV2: DropdownMenuRadioItem,
DropdownMenuItemLabelV2: DropdownMenuItemLabel,
DropdownMenuItemHelpTextV2: DropdownMenuItemHelpText,
DropdownMenuSeparatorV2: DropdownMenuSeparator,
} = unlock( componentsPrivateApis );

const useToolsPanelDropdownMenuProps = () => {
const isMobile = useViewportMatch( 'medium', '<' );
return ! isMobile
? {
popoverProps: {
placement: 'left-start',
// For non-mobile, inner sidebar width (248px) - button width (24px) - border (1px) + padding (16px) + spacing (20px)
offset: 259,
},
}
: {};
};
Comment on lines +41 to +52
Copy link
Contributor

Choose a reason for hiding this comment

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

I can see the useToolsPanelDropdownMenuProps is already defined the same way in a couple of places: Global Styles utils and Block Library utils.

After taking a quick look, it seems dropdownMenuProps is initialized using this util. I'm wondering if this should be part of the component, maybe the default or an option. Or have one unique util that can be used everywhere.

It doesn't make too much sense to me to replicate this code in all the places.

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 agree with that, but, at the same time, we would need to allow this option to be extensible and editable for external developers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pinging @WordPress/gutenberg-components because they will probably have more context if this makes sense or feasible.

Copy link
Contributor

@ciampo ciampo Jul 24, 2024

Choose a reason for hiding this comment

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

Thank you for the ping!

Those popover settings (placement and offset) are related to the layout of the block editor and the block inspector sidebar. The @wordpress/components package needs to provide UI components to more consumers than just the block editor, and therefore it should not be aware of those details otherwise it would become tightly coupled with the editor.

I agree though that it would be best to store those settings in a common place.

Since those settings are related to the sidebar, I would suggest storing them in a package that is a common dependency of the block editor, but not a generic package (like components, data, compose, gets).

Maybe in the @wordpress/interface package (where complementary area, the component used for the sidebar, is defined), or even in the @wordpress/editor package itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, thanks for the context! We can figure out the best place for this in a follow-up pull request I believe.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe in the @wordpress/interface package (where complementary area, the component used for the sidebar, is defined), or even in the @wordpress/editor package itself?

To my knowledge, there is intent to sunset @wordpress/interface in favor of @wordpress/editor.

Copy link
Contributor

Choose a reason for hiding this comment

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

I have created this follow-up pull request to use only one util and keep discussing it.


function BlockBindingsPanelDropdown( {
fieldsList,
addConnection,
attribute,
binding,
} ) {
const currentKey = binding?.args?.key;
return (
<>
{ Object.entries( fieldsList ).map( ( [ label, fields ], i ) => (
<Fragment key={ label }>
<DropdownMenuGroup>
{ Object.keys( fieldsList ).length > 1 && (
<Text
className="block-editor-bindings__source-label"
upperCase
variant="muted"
aria-hidden
>
{ label }
</Text>
) }
{ Object.entries( fields ).map( ( [ key, value ] ) => (
<DropdownMenuRadioItem
key={ key }
onChange={ () =>
addConnection( key, attribute )
}
name={ attribute + '-binding' }
value={ key }
checked={ key === currentKey }
>
<DropdownMenuItemLabel>
{ key }
</DropdownMenuItemLabel>
<DropdownMenuItemHelpText>
{ value }
</DropdownMenuItemHelpText>
</DropdownMenuRadioItem>
) ) }
</DropdownMenuGroup>
{ i !== Object.keys( fieldsList ).length - 1 && (
Copy link
Member

@gziolo gziolo Jul 25, 2024

Choose a reason for hiding this comment

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

This separator always was problematic as it rarely should be rendered after the last element. I dream of the future when there is a prop on the menu to automatically injects the separator when there are groups defined 😄

<DropdownMenuSeparator />
) }
</Fragment>
) ) }
</>
);
}

function BlockBindingsAttribute( { attribute, binding } ) {
const { source: sourceName, args } = binding || {};
const sourceProps =
unlock( blocksPrivateApis ).getBlockBindingsSource( sourceName );
return (
<VStack>
<Truncate>{ attribute }</Truncate>
{ !! binding && (
<Text
variant="muted"
className="block-editor-bindings__item-explanation"
>
<Truncate>
{ args?.key || sourceProps?.label || sourceName }
</Truncate>
Comment on lines +116 to +118
Copy link
Contributor

Choose a reason for hiding this comment

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

When we extract an API to add different sources, we should maybe consider how to let sources intentionally define a value here other than key, that way they could provide additional context if needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, we need to explore how other sources will hook into this. I don't know yet if using a key should be enforced or not and how much flexibility we should provide in this specific UI. They can always create their own UI for their use case if it defers from the Custom Fields one.

We can discuss that in a follow-up before opening the APIs.

Copy link
Member

Choose a reason for hiding this comment

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

In my opinion, key alone makes sense only as long we allow only Post Meta here. We should default to the name of the source, and show more details on subsequent action like hover. The alternative might be to offer an optional way for sources to define how the label is structured. There is something like that for block types which never got out of an experiment, example:

__experimentalLabel( attributes, { context } ) {
const { content, level } = attributes;
const customName = attributes?.metadata?.name;
const hasContent = content?.trim().length > 0;
// In the list view, use the block's content as the label.
// If the content is empty, fall back to the default label.
if ( context === 'list-view' && ( customName || hasContent ) ) {
return customName || content;
}
if ( context === 'accessibility' ) {
return ! hasContent
? sprintf(
/* translators: accessibility text. %s: heading level. */
__( 'Level %s. Empty.' ),
level
)
: sprintf(
/* translators: accessibility text. 1: heading level. 2: heading content. */
__( 'Level %1$s. %2$s' ),
level,
content
);
}
},

</Text>
) }
</VStack>
);
}

function ReadOnlyBlockBindingsPanelItems( { bindings } ) {
return (
<>
{ Object.entries( bindings ).map( ( [ attribute, binding ] ) => (
<Item key={ attribute }>
<BlockBindingsAttribute
attribute={ attribute }
binding={ binding }
/>
</Item>
) ) }
</>
);
}
Comment on lines +125 to +138
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add a visual indicator to let users know when the panel is read only? Otherwise the panel may just appear broken in those cases. I imagine this would be especially useful when a user doesn't have sufficient permissions to modify the bindings.

We could just add a gray background to the box. What do you think?

This PR is already big, so we could also handle that in a separate PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the UI for read only needs to be revisited, and I also agree it can be done in a follow-up.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could just add a gray background to the box. What do you think?
it can be done in a follow-up.

Agreed. Also, since this part of the UI is using ItemGroup and Item, it's a good opportunity to codify what a disabled Item looks like with the design folks, and add support for it directly to the component.

Please ping @WordPress/gutenberg-components when you start working on it!


function EditableBlockBindingsPanelItems( {
attributes,
bindings,
fieldsList,
addConnection,
removeConnection,
} ) {
const isMobile = useViewportMatch( 'medium', '<' );
return (
<>
{ attributes.map( ( attribute ) => {
const binding = bindings[ attribute ];
return (
<ToolsPanelItem
key={ attribute }
hasValue={ () => !! binding }
label={ attribute }
onDeselect={ () => {
removeConnection( attribute );
} }
>
<DropdownMenu
placement={
isMobile ? 'bottom-start' : 'left-start'
}
gutter={ isMobile ? 8 : 36 }
className="block-editor-bindings__popover"
trigger={
<Item>
<BlockBindingsAttribute
attribute={ attribute }
binding={ binding }
/>
</Item>
}
>
<BlockBindingsPanelDropdown
fieldsList={ fieldsList }
addConnection={ addConnection }
attribute={ attribute }
binding={ binding }
/>
</DropdownMenu>
</ToolsPanelItem>
);
} ) }
</>
);
}

export const BlockBindingsPanel = ( { name, metadata } ) => {
const registry = useRegistry();
const blockContext = useContext( BlockContext );
const { bindings } = metadata || {};
const { sources } = useSelect( ( select ) => {
const _sources = unlock(
select( blocksStore )
).getAllBlockBindingsSources();

return {
sources: _sources,
};
}, [] );

if ( ! bindings ) {
return null;
}
const bindableAttributes = getBindableAttributes( name );
const dropdownMenuProps = useToolsPanelDropdownMenuProps();

// Don't show not allowed attributes.
// Don't show the bindings connected to pattern overrides in the inspectors panel.
// TODO: Explore if this should be abstracted to let other sources decide.
const filteredBindings = { ...bindings };
Object.keys( filteredBindings ).forEach( ( key ) => {
if (
Expand All @@ -48,43 +205,129 @@ export const BlockBindingsPanel = ( { name, metadata } ) => {
}
} );

if ( Object.keys( filteredBindings ).length === 0 ) {
const { updateBlockAttributes } = useDispatch( blockEditorStore );

const { _id } = useSelect( ( select ) => {
const { getSelectedBlockClientId } = select( blockEditorStore );

return {
_id: getSelectedBlockClientId(),
};
}, [] );

if ( ! bindableAttributes || bindableAttributes.length === 0 ) {
return null;
}

const removeAllConnections = () => {
const newMetadata = { ...metadata };
delete newMetadata.bindings;
updateBlockAttributes( _id, {
metadata:
Object.keys( newMetadata ).length === 0
? undefined
: newMetadata,
} );
};

const addConnection = ( value, attribute ) => {
// Assuming the block expects a flat structure for its metadata attribute
const newMetadata = {
...metadata,
// Adjust this according to the actual structure expected by your block
bindings: {
...metadata?.bindings,
[ attribute ]: {
source: 'core/post-meta',
args: { key: value },
},
},
};
// Update the block's attributes with the new metadata
updateBlockAttributes( _id, {
metadata: newMetadata,
} );
};

const removeConnection = ( key ) => {
const newMetadata = { ...metadata };
if ( ! newMetadata.bindings ) {
return;
}

delete newMetadata.bindings[ key ];
if ( Object.keys( newMetadata.bindings ).length === 0 ) {
delete newMetadata.bindings;
}
updateBlockAttributes( _id, {
metadata:
Object.keys( newMetadata ).length === 0
? undefined
: newMetadata,
} );
};

const fieldsList = {};
const { getBlockBindingsSources } = unlock( blocksPrivateApis );
const registeredSources = getBlockBindingsSources();
Object.values( registeredSources ).forEach(
( { getFieldsList, label, usesContext } ) => {
if ( getFieldsList ) {
// Populate context.
const context = {};
if ( usesContext?.length ) {
for ( const key of usesContext ) {
context[ key ] = blockContext[ key ];
}
}
const sourceList = getFieldsList( {
registry,
context,
} );
// Only add source if the list is not empty.
if ( sourceList ) {
fieldsList[ label ] = { ...sourceList };
}
}
}
);

// At this moment, the UI can be locked when there are no fields to connect to.
const readOnly = ! Object.keys( fieldsList ).length;

if ( readOnly && Object.keys( filteredBindings ).length === 0 ) {
return null;
}

return (
<InspectorControls>
<PanelBody
title={ __( 'Attributes' ) }
className="components-panel__block-bindings-panel"
<ToolsPanel
label={ __( 'Attributes' ) }
resetAll={ () => {
removeAllConnections();
} }
dropdownMenuProps={ dropdownMenuProps }
className="block-editor-bindings__panel"
>
<BaseControl
help={ __( 'Attributes connected to various sources.' ) }
>
<ItemGroup isBordered isSeparated size="large">
{ Object.keys( filteredBindings ).map( ( key ) => {
return (
<Item key={ key }>
<HStack>
<span>{ key }</span>
<span className="components-item__block-bindings-source">
{ sources[
filteredBindings[ key ].source
]
? sources[
filteredBindings[ key ]
.source
].label
: filteredBindings[ key ]
.source }
</span>
</HStack>
</Item>
);
} ) }
</ItemGroup>
</BaseControl>
</PanelBody>
<ItemGroup isBordered isSeparated>
{ readOnly ? (
<ReadOnlyBlockBindingsPanelItems
bindings={ filteredBindings }
/>
) : (
<EditableBlockBindingsPanelItems
attributes={ bindableAttributes }
bindings={ filteredBindings }
fieldsList={ fieldsList }
addConnection={ addConnection }
removeConnection={ removeConnection }
/>
) }
</ItemGroup>
<Text variant="muted">
{ __( 'Attributes connected to various sources.' ) }
</Text>
</ToolsPanel>
</InspectorControls>
);
};
Expand Down
15 changes: 13 additions & 2 deletions packages/block-editor/src/hooks/block-bindings.scss
Original file line number Diff line number Diff line change
@@ -1,3 +1,14 @@
.components-panel__block-bindings-panel .components-item__block-bindings-source {
color: $gray-700;
div.block-editor-bindings__panel {
grid-template-columns: auto;
button:hover .block-editor-bindings__item-explanation {
Comment on lines +1 to +2
Copy link
Contributor

@SantosGuillamot SantosGuillamot Jul 25, 2024

Choose a reason for hiding this comment

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

For this use case, it seems we need to use just 1 column in the ToolsPanel component, but it seems it always uses 2 columns. @WordPress/gutenberg-components did you consider adding a new property to define the number of grid columns? This is something the Grid component is doing, for example.

Apart from that, I had to add more specificity to override those styles.

Copy link
Contributor

Choose a reason for hiding this comment

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

We were not as involved in the ToolsPanel component as on other components, and therefore we may lack some context on how the component layout works — but, looking at the component's docs, it seems like those decisions were taken deliberately.

My guess is that ToolsPanel was thought for a very specific case, and at the time it was not considered to allow more flexibility (perhaps on purpose, to ensure more coherency across the block editor's UI).

My advice is to follow the recommendations of the docs, and at most open a separate issue where we can discuss if and how to make ToolsPanel's layout more flexible (by allowing a custom number of columns? by allowing each item to specify a col span ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

So, instead of changing the wrapper to have just one column, we need to change the style of each item to fit both columns?

For reference, other examples where the "one-column" layout is used:

Colors
Screenshot 2024-07-30 at 08 45 45

Dimensions
Screenshot 2024-07-30 at 08 46 17

Filters
Screenshot 2024-07-30 at 08 46 02

Border & Shadow
Screenshot 2024-07-30 at 08 46 34

color: inherit;
}
}

.block-editor-bindings__popover {
// This won't be needed if `DropdownMenuGroup` component handles the label.
.block-editor-bindings__source-label {
grid-column: 2;
margin: $grid-unit-10 0;
}
}
Loading
Loading