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

Features like pattern overrides and block bindings have limited control over block UI #58233

Open
talldan opened this issue Jan 25, 2024 · 5 comments
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Block bindings [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.

Comments

@talldan
Copy link
Contributor

talldan commented Jan 25, 2024

What's the problem?

Several features require a way to manage specific block attributes and the UI controls for those attributes in a way that's difficult currently:

  • Block bindings (e.g. bindings to Custom fields) - At the time of writing a requirement is locking the editing of specific bound attributes and their UI controls, but in the future it's easy to see this evolving. Users may be able to directly update custom field values from a block control, and potentially create and see the status of bindings.
  • Pattern overrides - Currently the pattern block is making use of useBlockEditingMode( 'contentOnly' ) to manage the visibility of block controls. This hides the majority of block controls that are considered unrelated to content. The way contentOnly works is very inflexible—it completely hides the block inspector, even if there are content controls there (like the image block alt/title fields). It also doesn't do a great job of managing toolbar controls—lots of blocks have internal conditional logic for handling the visibility of controls based on the block editing mode, which becomes hard to manage. There's a problem in that the controls displayed or hidden are a fixed list that has no relation to the pattern overrides supported attributes. Changing the visibility of UI controls will also affect other usages of useBlockEditingMode( 'contentOnly' ) outside of patterns.

Offering better management over block attributes and controls might also tie into future initiatives around workflows and extensibility.

My feeling is that these problems are closely related, and this is an area that needs some exploration.

@talldan talldan added the [Type] Enhancement A suggestion for improvement. label Jan 25, 2024
@talldan
Copy link
Contributor Author

talldan commented Jan 25, 2024

I have some ideas about possible solutions to this, but wanted to keep the conversation open to other ideas, so didn't document it in the description.

One of the issues that we have right now is that our controls from the components package are only aware of values:

<TextControl value={ title } onChange={ onTitleChange } />

There's no awareness of the attribute declaration. I think we could consider changing this:

<TextControl attribute="title" value={ title } onChange={ onTitleChange } />

This second version of the control that has awareness of the attribute wouldn't come from the components package, but instead either the block editor package or a new package (e.g. 'block controls').

This package would re-export the existing components, but perform some extra business logic in the controls, for example, the 'block controls' version of TextControl:

import { TextControl } from '@wordpress/components';
import { useBlockBindingStatus, useBlockControlVisibility } from '@wordpress/block-editor';

export default function TextControl( { attribute, ...props } ) {
    const isVisible = useBlockControlVisibility( attribute );
    const bindingStatus = useBlockBindingStatus( attribute );
    
    if ( ! isVisible ) return null;
    
    return <TextControl { ...props } status={ bindingStatus } />;
}

I think this would help us avoid having to write complex logic within blocks.

The major downside is that the scope of this is pretty huge, gradually replacing the controls for blocks. Third parties could also adopt this but the process could be quite slow.

@gziolo
Copy link
Member

gziolo commented Jan 25, 2024

Excellent ideas @talldan. I have been talking to @SantosGuillamot about improving the current RichText implementation to abstract it further and account for the locking mechanism introduced in a very ad-hoc manner. I sketched some ideas as comments that are very close to what you shared for TextControl:

fd46752#r137777806

RichText has this optional identifier prop to pass the attribute name. As far as I know, it's used internally for text selections. Depending on the binding status, we could also use it to handle the state internally.

I also was thinking about other optimizations to improve the developer experience and better support the block bindings integration in #58085 (comment):

Instead of

<RichText
	tagName="pre"
	identifier="content"
	preserveWhiteSpace
	value={ content }
	onChange={ ( nextContent ) => {
		setAttributes( {
			content: nextContent,
		} );
	} }
	...   

we could have:

<RichText
	tagName="pre"
	attributeName="content"
	preserveWhiteSpace
	...   

so value and onChange could be automatically handled internally (when not provided) based on the attributeName (we could keep the identifier as name).

fd46752#r137780716

We also touched on a helper React hook that would simplify the check for binding status. This is what I drafted:

const [ lockUrlControls, lockAltControls, lockTitleControls ] = useBindingLockState( 'url',  'alt', 'title' );

function useBindingLockState( ...attributeNames ) ...

The major downside is that the scope of this is pretty huge, gradually replacing the controls for blocks. Third parties could also adopt this but the process could be quite slow.

I think that's a viable approach and worth the hassle!

@fabiankaegy
Copy link
Member

Something I've always been wondering about is how we made it work with content only locking.

I understand that there is a flag called __experimentalContent that one was able to use in attribute definitions that was then used to determine which controls to show / hide. But I never quite understood how the control itself was actually aware of being connected to that specific attribute. Because all the component knew was the current value and an anonymous function that intern called for the setAttributes function.

In theory I still quite like the API for having the definition for what a field is live with the attribute definition. But I never understood how that connection between the UI control and the attribute is made

@talldan
Copy link
Contributor Author

talldan commented Mar 19, 2024

In trunk right now, the entirety of the inspector controls are hidden when contentOnly is active, and then additionally some other controls have code like this to hide them visually:

if ( blockEditingMode !== 'default' ) {
return null;
}

So right now the onus is really on the implementor of the control to handle its visibility.

I think I'd like to see that inverted so that a UI control implicitly handles its own visibility.

@gaambo
Copy link
Contributor

gaambo commented Mar 19, 2024

I think block-developers implementing controls should always have the possibility, to define when their component should render - because a TexttControl might be used for some "content-related" attributes but maybe also for others (e.g. the field for custom CSS class names)? If I'm building a custom block, I might want to define "this control is content-related, this one is not".

But on the other side, as a developer using core/3rd party blocks, I would want to overwrite those conditions sometimes - for example based on attribute bindings, the permissions of the user (to make permissions in editing more granular), etc.

Having the role (e.g. __experimentalContent) on the attribute doesn't always make sense either - because a string attribute may be content, but have a binding (or pattern override) and therefore still shouldn't be shown. Also, we have to think about object-type attributes - like the queryArgs in the core/query block. There's just one attribute, but all the sub-properties are connected to single controls and those controls can be enabled/disabled in variations. Inheriting the query is kind of like an "edit-mode" here as well - because all the controls are disabled them.

I always thought that the "structureless" rendering of controls inside in React makes it hard to disable/enable only some of them. E.g. via SlotFills one can actually only add controls, but never remove controls (so kind of like the concepts of do_action vs. apply_filters).

Those are just thoughts, I have no concrete useful input/ideas 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Feature] Block bindings [Package] Block editor /packages/block-editor [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

4 participants