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 all 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
81 changes: 41 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,8 +20,9 @@ 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';
import { __, sprintf } from '@wordpress/i18n';

/**
* Internal dependencies
Expand Down Expand Up @@ -163,49 +164,47 @@ 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 relatedBinding = blockBindings[ identifier ];
const { getBlockBindingsSource } = unlock( select( blocksStore ) );
const blockBindingsSource = getBlockBindingsSource(
relatedBinding.source
);

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

const _bindingsPlaceholder = _disableBoundBlock
? relatedBinding?.args?.key || blockBindingsSource?.label
: sprintf(
/* translators: %s: source label or key */
__( 'Add %s' ),
relatedBinding?.args?.key || blockBindingsSource?.label
);

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

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

const { getSelectionStart, getSelectionEnd, getBlockRootClientId } =
useSelect( blockEditorStore );
Expand Down Expand Up @@ -335,7 +334,7 @@ export function RichTextWrapper(
selectionStart,
selectionEnd,
onSelectionChange,
placeholder,
placeholder: bindingsPlaceholder || placeholder,
__unstableIsSelected: isSelected,
__unstableDisableFormats: disableFormats,
preserveWhiteSpace,
Expand Down Expand Up @@ -404,9 +403,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