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

Jump to bottom

Block Bindings: Prioritize existing placeholder over bindingsPlaceholder #65220

Merged
merged 11 commits into from
Sep 24, 2024
Merged

Block Bindings: Prioritize existing placeholder over bindingsPlaceholder #65220

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
5863a0f
Pass data-custom-placeholder prop to all RichText instances that are …
zaguiini Sep 20, 2024
a816522
Use placeholder if data-custom-placeholder is true
zaguiini Sep 20, 2024
f9e53bf
Add test
zaguiini Sep 20, 2024
394e032
Only use placeholder if aria-label is not defined
zaguiini Sep 20, 2024
4be800d
Prioritize custom placeholder in aria-label
zaguiini Sep 20, 2024
e71d384
Fix Cover block test
zaguiini Sep 20, 2024
9257d80
Revert aria-label changes
zaguiini Sep 20, 2024
fa19fc8
Try: Read block attribute in bindings placeholder
SantosGuillamot Sep 20, 2024
eb71d51
Revert changes to placeholder prop
SantosGuillamot Sep 20, 2024
a9da96d
Fix after rebase
SantosGuillamot Sep 20, 2024
0027219
Explicitly return `null` for empty values
SantosGuillamot Sep 24, 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
32 changes: 24 additions & 8 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 @@ -165,7 +165,7 @@ export function RichTextWrapper(
isBlockSelected,
] );

const { disableBoundBlock, bindingsPlaceholder } = useSelect(
const { disableBoundBlock, bindingsPlaceholder, bindingsLabel } = useSelect(
( select ) => {
if (
! blockBindings?.[ identifier ] ||
Expand All @@ -179,10 +179,6 @@ export function RichTextWrapper(
const blockBindingsSource = getBlockBindingsSource(
relatedBinding.source
);
const fieldsList = blockBindingsSource?.getFieldsList?.( {
registry,
context: blockContext,
} );

const _disableBoundBlock =
! blockBindingsSource?.canUserEditValue?.( {
Expand All @@ -191,6 +187,19 @@ export function RichTextWrapper(
args: relatedBinding.args,
} );

// Don't modify placeholders if value is not empty.
if ( adjustedValue.length > 0 ) {
return {
disableBoundBlock: _disableBoundBlock,
Copy link
Member

Choose a reason for hiding this comment

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

Interesting, should it explicitly return null for bindingsPlaceholder and bindingsLabel with some more detailed explanation?

Alternatively, would the following work and made it easier to follow:

Suggested change
disableBoundBlock: _disableBoundBlock,
disableBoundBlock: _disableBoundBlock,
bindingsLabel: props[ 'aria-label' ] || placeholder,

Copy link
Member

Choose a reason for hiding this comment

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

A similar trick might also work for bindingsPlaceholder.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've explicitly return null as suggested. I kind of like having the conditionals outside of the bindings logic. It makes it clear when the placeholder and the aria-label are defined:

Having said that, I don't have a strong opinion so I'm happy to change that.

};
}

const { getBlockAttributes } = select( blockEditorStore );
const blockAttributes = getBlockAttributes( clientId );
const fieldsList = blockBindingsSource?.getFieldsList?.( {
registry,
context: blockContext,
} );
const bindingKey =
fieldsList?.[ relatedBinding?.args?.key ]?.label ??
blockBindingsSource?.label;
Expand All @@ -202,12 +211,19 @@ export function RichTextWrapper(
__( 'Add %s' ),
bindingKey
);
const _bindingsLabel = _disableBoundBlock
? relatedBinding?.args?.key || blockBindingsSource?.label
: sprintf(
/* translators: %s: source label or key */
__( 'Empty %s; start writing to edit its value' ),
relatedBinding?.args?.key || blockBindingsSource?.label
);

return {
disableBoundBlock: _disableBoundBlock,
bindingsPlaceholder:
( ! adjustedValue || adjustedValue.length === 0 ) &&
_bindingsPlaceholder,
blockAttributes?.placeholder || _bindingsPlaceholder,
bindingsLabel: _bindingsLabel,
};
},
[
Expand Down Expand Up @@ -421,7 +437,7 @@ export function RichTextWrapper(
aria-readonly={ shouldDisableEditing }
{ ...props }
aria-label={
bindingsPlaceholder || props[ 'aria-label' ] || placeholder
gziolo marked this conversation as resolved.
Show resolved Hide resolved
bindingsLabel || props[ 'aria-label' ] || placeholder
}
{ ...autocompleteProps }
ref={ useMergeRefs( [
Expand Down
35 changes: 34 additions & 1 deletion test/e2e/specs/editor/various/block-bindings.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1278,6 +1278,39 @@ test.describe( 'Block bindings', () => {
).toHaveText( 'fallback value' );
} );

test( 'should prioritize the placeholder attribute over the placeholder generated by the bindings API', async ( {
editor,
} ) => {
await editor.insertBlock( {
name: 'core/paragraph',
attributes: {
placeholder: 'My custom placeholder',
content: 'paragraph default content',
metadata: {
bindings: {
content: {
source: 'core/post-meta',
args: { key: 'empty_field' },
},
},
},
},
} );

const paragraphBlock = editor.canvas.getByRole( 'document', {
// Aria-label is changed for empty paragraphs.
name: 'Empty empty_field; start writing to edit its value',
} );

await expect( paragraphBlock ).toBeEmpty();

const placeholder = paragraphBlock.locator( 'span' );
await expect( placeholder ).toHaveAttribute(
'data-rich-text-placeholder',
'My custom placeholder'
);
} );

test( 'should show the prompt placeholder in field with empty value', async ( {
editor,
} ) => {
Expand All @@ -1298,7 +1331,7 @@ test.describe( 'Block bindings', () => {

const paragraphBlock = editor.canvas.getByRole( 'document', {
// Aria-label is changed for empty paragraphs.
name: 'Add empty_field',
name: 'Empty empty_field; start writing to edit its value',
} );

await expect( paragraphBlock ).toBeEmpty();
Expand Down
Loading