-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: allow the field types matching attribute types in bindings. #66174
Block bindings: allow the field types matching attribute types in bindings. #66174
Conversation
Size Change: +74 B (0%) Total Size: 1.77 MB
ℹ️ View Unchanged
|
// Don't include private fields. | ||
key.charAt( 0 ) !== '_' && | ||
// Only support string types. | ||
typeof entityMetaValues?.[ key ] === 'string' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could add the filter here:
const metaProperties =
options?.schema?.properties?.meta?.properties;
const stringMetaProperties = Object.fromEntries(
Object.entries( metaProperties ).filter(
( [ , value ] ) => value.type === 'string'
)
);
dispatch.receiveRegisteredPostMeta(
postType,
stringMetaProperties
);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I think it makes more sense to have the check here in packages/editor/src/bindings/post-meta.js
. That makes the check more specific to bindings and keeps the related exceptions together, whereas getRegisteredPostMeta
seems better to me conceptually as a more agnostic function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the conditional should look like this, adding some handling for default values, otherwise valid fields will get skipped over in templates where entityMetaFields
is undefined due to postId
being undefined. Consequently, this also fixes some broken tests.
It might cause other things to break, though — I haven't been able to confirm that yet:
if (
// Don't include footnotes.
key !== 'footnotes' &&
// Don't include private fields.
key.charAt( 0 ) !== '_' &&
// Only support string types.
(typeof entityMetaValues?.[ key ] === 'string' || typeof props.default === 'string')
) { // logic here }
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree getRegisteredPostMeta
should be agnostic to bindings and in other case an object or an array could be a valid value. I want to explore if it should happen in the bindings hooks instead of just post meta.
And it's true that we can't just check against string
because undefined
is also a valid value. I'll spend some time today exploring the best way to handle this.
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
@@ -322,7 +322,7 @@ test.describe( 'Post Meta source', () => { | |||
// Check the post meta fields are not visible. | |||
const globalField = page | |||
.getByRole( 'menuitemradio' ) | |||
.filter( { hasText: 'text_custom_field' } ); | |||
.filter( { hasText: 'Text custom field' } ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated some tests here due to the label property now being supplied in packages/e2e-tests/plugins/block-bindings.php
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related to this, I believe text_custom_field
in the following test should also be rendering as Text custom field
, but it's not and I'm not sure why:
gutenberg/test/e2e/specs/editor/various/block-bindings/post-meta.spec.js
Lines 332 to 355 in 1bb2779
test( 'should show the key in attributes connected to post meta', async ( { | |
editor, | |
} ) => { | |
await editor.insertBlock( { | |
name: 'core/paragraph', | |
attributes: { | |
content: 'fallback content', | |
metadata: { | |
bindings: { | |
content: { | |
source: 'core/post-meta', | |
args: { | |
key: 'text_custom_field', | |
}, | |
}, | |
}, | |
}, | |
}, | |
} ); | |
const paragraphBlock = editor.canvas.getByRole( 'document', { | |
name: 'Block: Paragraph', | |
} ); | |
await expect( paragraphBlock ).toHaveText( 'text_custom_field' ); | |
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is because that test is part of the Custom Template tests, which doesn't support post meta yet (because we aren't able to get only the fields that apply to any post type).
Thinking about it, maybe it is better to create a new "String custom field"? We were using "text custom field" to test that the key is shown when there is no label, and that's a valid use case.
I’ve been triaging it and it is a problem of how the object custom field is registered. The default property is not valid. It needs to include a schema with the properties. And I am not sure if it is possible to define a default for an object type (I haven't been able to). I’ve updated the test example to reflect that. |
Cross-checking that would be the best for now. At least for the UI to select Post Meta filtring out post meta that doesn’t match the same type as the attribute would be ideal. |
All the types used for supported core blocks and their attributes with the
*: not covered yet. So, there is the possibility of setting the image's My previous point #66174 (comment) holds. The best approach would require applying the following changes:
|
I made a first attempt for this in this commit: 98e41bd My idea was to add a new |
select( blockEditorStore ).getBlock( clientId ); | ||
const _attributeType = | ||
getBlockType( blockName ).attributes?.[ attribute ]?.type; | ||
return _attributeType === 'rich-text' ? 'string' : _attributeType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will need a more sophisticated utility in the future. See my notes in https://github.com/WordPress/gutenberg/pull/66174/files#r1804482542. The type can list multiple types, too.
We can split that into two PRs and focus on the connections UI which we know causes the block to crash when there is type mismatch. |
Yes, maybe that's better. I did a quick test and it seems the paragraph content, for example, doesn't work as expected when set a number as its value: wp.data.dispatch( 'core/block-editor' ).updateBlockAttributes(
'ce3120bd-57d6-405b-ae47-e159df59c6cb',
{
content: 5,
}
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code-wise looks good. It would be great to have someone running some additional rounds of manual testing.
Co-authored-by: Greg Ziółkowski <[email protected]>
Flaky tests detected in 1512311. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11405532512
|
* Filter fields by type in post meta * Add e2e test * Allow only strings * Update test to expect numbers and integers to be hidden * Update tests to check for label instead of key * Adapt object custom field * Adapt e2e tests * Add `type` prop to `getFieldsList` * Adapt testing source * Reduce diff * Add `attribute` to dependencies --------- Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: cbravobernal <[email protected]> Co-authored-by: artemiomorales <[email protected]> Co-authored-by: gziolo <[email protected]>
I just cherry-picked this PR to the wp/6.7 branch to get it included in the next release: 60306c5 |
I did a manual testing and works as expected. |
It works well for me too 🙌 |
With the latest implementation, it is not exactly that the UI only supports string and rich-text. It currently maps the attribute type with the field type. What happens is that most of the attributes supported right now are supposed to be strings. For example, if I am not mistaken, in the image id you should be able to connect a "number". |
Ok got it, thanks! I thought that saying only strings or rich-text would be a simple way of explaining it, but I do see the image id needs to be connected to a number. |
* Filter fields by type in post meta * Add e2e test * Allow only strings * Update test to expect numbers and integers to be hidden * Update tests to check for label instead of key * Adapt object custom field * Adapt e2e tests * Add `type` prop to `getFieldsList` * Adapt testing source * Reduce diff * Add `attribute` to dependencies --------- Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: cbravobernal <[email protected]> Co-authored-by: artemiomorales <[email protected]> Co-authored-by: gziolo <[email protected]>
I just cherry-picked this PR to the release/19.5 branch to get it included in the next release: da326f9 |
…#66174) * Filter fields by type in post meta * Add e2e test * Allow only strings * Update test to expect numbers and integers to be hidden * Update tests to check for label instead of key * Adapt object custom field * Adapt e2e tests * Add `type` prop to `getFieldsList` * Adapt testing source * Reduce diff * Add `attribute` to dependencies --------- Co-authored-by: SantosGuillamot <[email protected]> Co-authored-by: cbravobernal <[email protected]> Co-authored-by: artemiomorales <[email protected]> Co-authored-by: gziolo <[email protected]>
I still want to:
updateBlockAttributes
handle this.What?
Fix a bug where the Attributes panel breaks the block when there are fields with type "object" or "array" defined.
Why?
This is not the expected behavior.
How?
Restricting the type of fields that can be used.
Testing Instructions
I added a e2e test.