-
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
Changes from 5 commits
3c8f338
c21e55a
ece9a68
02bcdc4
6f8397c
3ef22f9
48485b1
98e41bd
346faed
41d6aba
1512311
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -81,7 +81,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
const paragraphBlock = editor.canvas.getByRole( 'document', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'Block: Paragraph', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( paragraphBlock ).toHaveText( | ||||||||||||||||||||||||||||||||||||||||||||||||||
Check failure on line 84 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:62:4 › Post Meta source › Movie CPT template › Block attributes values › should show the default value if it is defined
Check failure on line 84 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:62:4 › Post Meta source › Movie CPT template › Block attributes values › should show the default value if it is defined
Check failure on line 84 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:62:4 › Post Meta source › Movie CPT template › Block attributes values › should show the default value if it is defined
|
||||||||||||||||||||||||||||||||||||||||||||||||||
'Movie field default value' | ||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -107,7 +107,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
const paragraphBlock = editor.canvas.getByRole( 'document', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'Block: Paragraph', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( paragraphBlock ).toHaveText( | ||||||||||||||||||||||||||||||||||||||||||||||||||
Check failure on line 110 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:88:4 › Post Meta source › Movie CPT template › Block attributes values › should fall back to the field label if the default value is not defined
Check failure on line 110 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:88:4 › Post Meta source › Movie CPT template › Block attributes values › should fall back to the field label if the default value is not defined
Check failure on line 110 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:88:4 › Post Meta source › Movie CPT template › Block attributes values › should fall back to the field label if the default value is not defined
|
||||||||||||||||||||||||||||||||||||||||||||||||||
'Field with only label' | ||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -166,7 +166,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
await page | ||||||||||||||||||||||||||||||||||||||||||||||||||
.getByRole( 'menuitemradio' ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
.filter( { hasText: 'Movie field label' } ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
.click(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
Check failure on line 169 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:143:4 › Post Meta source › Movie CPT template › Attributes panel › should show the field label if it is defined
Check failure on line 169 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:143:4 › Post Meta source › Movie CPT template › Attributes panel › should show the field label if it is defined
Check failure on line 169 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:143:4 › Post Meta source › Movie CPT template › Attributes panel › should show the field label if it is defined
|
||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( contentBinding ).toContainText( | ||||||||||||||||||||||||||||||||||||||||||||||||||
'Movie field label' | ||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -197,7 +197,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
await page | ||||||||||||||||||||||||||||||||||||||||||||||||||
.getByRole( 'menuitemradio' ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
.filter( { hasText: 'field_without_label_or_default' } ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
.click(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
Check failure on line 200 in test/e2e/specs/editor/various/block-bindings/post-meta.spec.js GitHub Actions / Playwright - 3[chromium] › editor/various/block-bindings/post-meta.spec.js:174:4 › Post Meta source › Movie CPT template › Attributes panel › should fall back to the field key if the field label is not defined
|
||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( contentBinding ).toContainText( | ||||||||||||||||||||||||||||||||||||||||||||||||||
'field_without_label_or_default' | ||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -236,14 +236,14 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
} ) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
const globalField = page | ||||||||||||||||||||||||||||||||||||||||||||||||||
.getByRole( 'menuitemradio' ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
.filter( { hasText: 'text_custom_field' } ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
.filter( { hasText: 'Text custom field' } ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( globalField ).toBeVisible(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
test( 'should not include protected fields', async ( { page } ) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
// Ensure the fields have loaded by checking the field is visible. | ||||||||||||||||||||||||||||||||||||||||||||||||||
const globalField = page | ||||||||||||||||||||||||||||||||||||||||||||||||||
.getByRole( 'menuitemradio' ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
.filter( { hasText: 'text_custom_field' } ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
.filter( { hasText: 'Text custom field' } ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( globalField ).toBeVisible(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
// Check the protected fields are not visible. | ||||||||||||||||||||||||||||||||||||||||||||||||||
const protectedField = page | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -322,7 +322,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
// 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to this, I believe gutenberg/test/e2e/specs/editor/various/block-bindings/post-meta.spec.js Lines 332 to 355 in 1bb2779
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. |
||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( globalField ).toBeHidden(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
const movieField = page | ||||||||||||||||||||||||||||||||||||||||||||||||||
.getByRole( 'menuitemradio' ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -547,5 +547,54 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||
.filter( { hasText: 'Movie field label' } ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( movieField ).toBeVisible(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
test( 'should not be possible to connect non-supported fields through the attributes panel', async ( { | ||||||||||||||||||||||||||||||||||||||||||||||||||
editor, | ||||||||||||||||||||||||||||||||||||||||||||||||||
page, | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||
await editor.insertBlock( { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'core/paragraph', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await page.getByLabel( 'Attributes options' ).click(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await page | ||||||||||||||||||||||||||||||||||||||||||||||||||
.getByRole( 'menuitemcheckbox', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'Show content', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
.click(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await page | ||||||||||||||||||||||||||||||||||||||||||||||||||
.getByRole( 'button', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'content', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
.click(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||
page.getByRole( 'menuitemradio', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'Text custom field', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
).toBeVisible(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||
page.getByRole( 'menuitemradio', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'Number custom field', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
).toBeHidden(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||
page.getByRole( 'menuitemradio', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'Integer custom field', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
).toBeHidden(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||
page.getByRole( 'menuitemradio', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'Boolean custom field', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
).toBeHidden(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||
page.getByRole( 'menuitemradio', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'Object custom field', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
).toBeHidden(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
await expect( | ||||||||||||||||||||||||||||||||||||||||||||||||||
page.getByRole( 'menuitemradio', { | ||||||||||||||||||||||||||||||||||||||||||||||||||
name: 'Array custom field', | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ) | ||||||||||||||||||||||||||||||||||||||||||||||||||
).toBeHidden(); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); | ||||||||||||||||||||||||||||||||||||||||||||||||||
} ); |
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:
https://github.com/WordPress/gutenberg/blob/0010f6355a37b647f2246ddbd88bba886a84a9cb/packages/core-data/src/resolvers.js#L1011-L1018
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, whereasgetRegisteredPostMeta
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 topostId
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:
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
becauseundefined
is also a valid value. I'll spend some time today exploring the best way to handle this.