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

Block Bindings: Change placeholder when attribute is bound #64903

Merged
merged 8 commits into from
Sep 2, 2024
Merged

Block Bindings: Change placeholder when attribute is bound #64903

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
eb2c262
Change placeholder when attribute is bound
SantosGuillamot Aug 29, 2024
96580c6
Only change placeholder when value is empty
SantosGuillamot Aug 29, 2024
e637c1c
Translate empty text
SantosGuillamot Aug 30, 2024
6cde7b0
Adapt latest commit
SantosGuillamot Aug 30, 2024
14fc2f2
Add `adjustedValue` in `useSelect`
SantosGuillamot Aug 30, 2024
5806c0a
Different message depending on capabilities
SantosGuillamot Aug 30, 2024
daba2d6
Remove "empty" from placeholder
SantosGuillamot Sep 2, 2024
eedbb73
Change `relatedBinding` variable name
SantosGuillamot Sep 2, 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
76 changes: 36 additions & 40 deletions packages/block-editor/src/components/rich-text/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {
removeFormat,
} from '@wordpress/rich-text';
import { Popover } from '@wordpress/components';
import { getBlockType, store as blocksStore } from '@wordpress/blocks';
import { store as blocksStore } from '@wordpress/blocks';
import deprecated from '@wordpress/deprecated';

/**
Expand Down Expand Up @@ -163,49 +163,43 @@ export function RichTextWrapper(
isBlockSelected,
] );

const disableBoundBlocks = useSelect(
const { disableBoundBlock, bindingsPlaceholder } = useSelect(
( select ) => {
// Disable Rich Text editing if block bindings specify that.
let _disableBoundBlocks = false;
if ( blockBindings && canBindBlock( blockName ) ) {
const blockTypeAttributes =
getBlockType( blockName ).attributes;
const { getBlockBindingsSource } = unlock(
select( blocksStore )
);
for ( const [ attribute, binding ] of Object.entries(
blockBindings
) ) {
if (
blockTypeAttributes?.[ attribute ]?.source !==
'rich-text'
) {
break;
}

// If the source is not defined, or if its value of `canUserEditValue` is `false`, disable it.
const blockBindingsSource = getBlockBindingsSource(
binding.source
);
if (
! blockBindingsSource?.canUserEditValue?.( {
select,
context: blockContext,
args: binding.args,
} )
) {
_disableBoundBlocks = true;
break;
}
}
if (
! blockBindings?.[ identifier ] ||
! canBindBlock( blockName )
) {
return {};
}

return _disableBoundBlocks;
const relevantBinding = blockBindings[ identifier ];
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't really like the name, but cannot find any better. Why is relevant?

Copy link
Contributor

Choose a reason for hiding this comment

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

I find relevantBinding to be unclear.

Maybe relatedBinding could work? Another option is associatedBinding.
If neither of those, honestly for me just binding would work fine and might even be the clearest. Another possibility is richTextBinding (my least favorite, but could work).

Other ideas: connectedBinding, targetBinding, linkedBinding, though I like these options less and they also seem unclear to me.

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 can use relatedBinding 👍

const { getBlockBindingsSource } = unlock( select( blocksStore ) );
const blockBindingsSource = getBlockBindingsSource(
relevantBinding.source
);

const _disableBoundBlock =
! blockBindingsSource?.canUserEditValue?.( {
select,
context: blockContext,
args: relevantBinding.args,
} );

const _bindingsPlaceholder =
( relevantBinding?.args?.key || blockBindingsSource?.label ) +
' value is empty';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It'd be great to get feedback on the desired text.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think is fine, maybe we can remove value.

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 thought about removing it, but when using the source label felt a bit weird:

  • "my_custom_field is empty" sounds fine.
  • "Custom Source is empty" doesn't sound as good to me.

Having said that, I don't have a strong opinion on this.

Copy link
Contributor

Choose a reason for hiding this comment

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

Me neither 🤷‍♂️

SantosGuillamot marked this conversation as resolved.
Show resolved Hide resolved

return {
disableBoundBlock: _disableBoundBlock,
bindingsPlaceholder:
( ! adjustedValue || adjustedValue.length === 0 ) &&
_bindingsPlaceholder,
};
},
[ blockBindings, blockName ]
[ blockBindings, identifier, blockName, blockContext ]
);

const shouldDisableEditing = readOnly || disableBoundBlocks;
const shouldDisableEditing = readOnly || disableBoundBlock;

const { getSelectionStart, getSelectionEnd, getBlockRootClientId } =
useSelect( blockEditorStore );
Expand Down Expand Up @@ -335,7 +329,7 @@ export function RichTextWrapper(
selectionStart,
selectionEnd,
onSelectionChange,
placeholder,
placeholder: bindingsPlaceholder || placeholder,
__unstableIsSelected: isSelected,
__unstableDisableFormats: disableFormats,
preserveWhiteSpace,
Expand Down Expand Up @@ -404,9 +398,11 @@ export function RichTextWrapper(
// Overridable props.
role="textbox"
aria-multiline={ ! disableLineBreaks }
aria-label={ placeholder }
aria-readonly={ shouldDisableEditing }
{ ...props }
aria-label={
bindingsPlaceholder || props[ 'aria-label' ] || placeholder
Copy link
Contributor

Choose a reason for hiding this comment

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

is placeholder already defaulted on line 337?

Copy link
Contributor

Choose a reason for hiding this comment

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

is placeholder already defaulted on line 337?

+1 — I think having just aria-label={ placeholder || props[ 'aria-label' ] } should be good.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The problem is the priority. props[ 'aria-label' ] is supposed to override the default placeholder value if it exist for the aria-label attribute. And I believe we want to override the aria-label prop when a binding exists.

Copy link
Member

Choose a reason for hiding this comment

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

@zaguiini, regarding your comment from #65089 (review).

Another issue, though related, is that the Block Bindings API does not respect an existing placeholder attribute. From the back-end, I'm setting a bespoke placeholder:

I think this is what changed the behavior, as placeholder became here a fallback. Now, the question is whether there is always placeholder coming from the server. If not, then we should be fine reverting to the old behavior and including the fallback to bindingsPlaceholder when the RichText is connected to a source.

}
{ ...autocompleteProps }
ref={ useMergeRefs( [
// Rich text ref must be first because its focus listener
Expand Down
Loading