-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 editing in post meta source #61753
Block Bindings: Allow editing in post meta source #61753
Conversation
2a71c24
to
18e6a5c
Compare
Size Change: +264 B (+0.02%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
292b7da
to
fae903b
Compare
There is an issue when pressing "Enter" to add a new block, and the rich text is not disabled. I'll take a look at that. |
I think the root of the problem is that when we call Not sure if modifying the attributes in the store itself instead of using the current hook would help in this regard. |
995607a
to
b3f4488
Compare
I have rebased this branch into this other pull request because it is supposed to solve the issues I was encountering while pressing Enter on bound blocks. |
}, | ||
} ); | ||
}, | ||
lockAttributesEditing( { select, context, args } ) { |
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 was a bit confused when I saw this method name as I expected it's an action that locks attribute editing. What alternatives would help here to better express intent – check whether the connected value can be updated?
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.
You're right it can be confusing. We could change the intent to just the opposite. Some ideas that come to my mind:
allowEditing
canEditSource
canUpdateSource
canUpdateSourceValue
Any ideas are welcome 🙂
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.
One important consideration is that the user might be able to edit source, but not necessarily all posts or individual fields, as permissions are very granular. I don't know if that gets reflected on the client.
I see that permissions are checked only on the post level with canUserEditEntityRecord
. In effect, canUserEditValue
or canEditValue
might resonate better.
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've changed the name to canUserEditValue
in the latest commit. It looks better to me. Anyways, the API is still private so we can change that if wanted.
One important consideration is that the user might be able to edit source, but not necessarily all posts or individual fields, as permissions are very granular. I don't know if that gets reflected on the client.
Do you mean that the current conditions might not be enough? We are assuming that if these conditions are true
, the user can edit the meta field:
- The user can edit the current post.
- The meta field is publicly exposed in the REST API.
What else should we consider?
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 was thinking about the is_protected_meta
function that allows filtering, but you are correct that when the individual meta fields gets exposed through REST API for the current user (and their role) then we can safely assume they can edit it 👍🏻
ed11e1a
to
3165915
Compare
5fdbacb
to
8e6dd67
Compare
Flaky tests detected in 570a4b2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9251566900
|
c8fd783
to
629056c
Compare
570a4b2
to
d35db11
Compare
9bfc757
to
7f8771e
Compare
2012ae8
to
c72afda
Compare
Did some testing, I'm not seeing any issues 👍 |
registry.dispatch( noticesStore ); | ||
createWarningNotice( | ||
__( | ||
"Blocks can't be inserted into other blocks with bindings" |
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.
You can paste a paragraph inside a binded paragraph to update its content right?
I could do that on my testing, so in that case, we may change the warning to
Blocks can't be insterted into different blocks with bindings
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.
You can paste a paragraph inside a binded paragraph to update its content right?
Mmm, I don't think so. You can paste some text, but not if you copy the entire block I believe.
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.
In my testing, it largely depend when you try to paste blocks. For example, try it with the Heading block, the Verse block, or caption for the Image block. It will get handled differently without any notices in the UI:
Screen.Recording.2024-05-31.at.12.54.22.mov
In effect, my recommendation would be to take some action that fits best in the given scenario and skip the warning.
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.
Ideally, we probably want to modify the source value with whatever is supposed to be pasted. But that seemed a bit tricky to me at this point and I thought it could be handled in a follow-up PR. That's why I added the warning in the meantime.
); | ||
return; | ||
} | ||
dispatch.insertAfterBlock( selectionA.clientId ); |
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.
dispatch.insertAfterBlock( selectionA.clientId ); |
I would remove the enter
option, is something we can add in future releases after some UX research.
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.
Works as expected. Apart from the enter
behaviour. The PR is ready to land.
I found that if you edit the custom field meta box, the editing stops from working.
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 seems that the custom fields edit panel never worked well with bindings, so should not be a block for this PR.
}, | ||
} ); | ||
}, | ||
canUserEditValue( { select, context, args } ) { |
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.
This is all private right?
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.
Yes, registering block binding sources is still private. If everything goes well in WP 6.6 cycle, we are going to open it to everyone in WP 6.7.
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.
Yes, and we will use next cycle to polish the APIs. There are some changes I would like to explore.
Everything works correctly in my testing. It looks like permissions are respected, so when editing the post as an author, when I use Query block to display other posts, I can't modify meta values for fields I don't have access to: Screen.Recording.2024-05-31.at.13.17.36.movOverall, I can see only two aspects to watch after this very exciting feature gets enabled for users:
Screen.Recording.2024-05-31.at.13.26.20.mov |
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.
Excellent work bringing it to the finish line. This is extremely exciting to see how easy it becomes to edit post meta inline 🎉
I did a very extensive testing using the admin and author roles, all supported block types: Paragraph, Heading, Button and Image, product and site editor. I couldn't discover any blockers. I would be in favor of including it in WordPress 6.6 release.
Thanks a lot for the extensive testing, Greg! Much appreciated I totally agree that we will need to keep iterating on the "Enter" and "Paste" workflows, so let's keep an eye on that. Also, once it is out we will get community feedback that can help make decisions.
Regarding this, I just created this pull request to remove them from the bindings panel. |
* Create bindings util for transforming block attributes * Get attributes and name from the store * Change function to return only the bound attributes * Add action, selector, and reducer for block context * Sync store in edit component * Revert "Sync store in edit component" This reverts commit 988c4b6. * Move logic to BlockContextProvider * Change parent context logic * Use useLayoutEffect * Go back to syncing store in edit component * WIP: Move bindings logic to `getBlockAttributes` * WIP: Move bindings setAttributes logic to updateBlockBindings * Pass only `select` to `getValue` functions * Remove old editor hook * Add fallback to postId until context is ready * Remove setValue post-meta code * Simplify fallback conditional * Change bindings destructuring * Check canBindAttribute in updateBlockAttributes * Update unit tests to expect a dispatch * Add conditional in block * Don't use `getBlockAttributes` inside `getValue` * Revert "Don't use `getBlockAttributes` inside `getValue`" This reverts commit 0e91129. * Avoid processing bindings recursively * Access context through selector * Update getBlockAttributes logic * Don't use fallbacks * Add edit value posibility for post meta, add function to check if is admin * Revert "Add edit value posibility for post meta, add function to check if is admin" This reverts commit 9659455. * Test editing is allowed in paragraph block * Test protected fields are not editable * Revert "Enable parallel processing for PHPCS sniffs (WordPress#61700)" This reverts commit 8331820. * Add post meta setValue function * Update tests to check contenteditable * Update lockAttributesEditing default * Pass arguments to lockAttributesEditing * Check user can edit post meta * Check field is exposed in the REST API * Disable editing in templates * Add fallback for postId * Revert "Revert "Enable parallel processing for PHPCS sniffs (WordPress#61700)"" This reverts commit e74d71c. * Adapt old tests * Simplify lockAttributesEditing fallbacks * Don't use fallback for context * Add postType fallback when locking controls * Pass context in rich text * Check contenteditable attribute in test * Change name to `canUserEditValue` * Revert changes caused by rebasing * Add space back * Pass block context through rich text * Change imports * Transform block attributes into bindings in split selection * Pass context to in use-input * Change REST API check * Use getBoundAttributesValues * Don't split when attribute is bound * Revert changes caused by rebase * Cover more blocks when editing custom fields * Add a warning when pasting blocks --------- Co-authored-by: Carlos Bravo <carlos.bravo@automattic.com>
* Create bindings util for transforming block attributes * Get attributes and name from the store * Change function to return only the bound attributes * Add action, selector, and reducer for block context * Sync store in edit component * Revert "Sync store in edit component" This reverts commit 988c4b6. * Move logic to BlockContextProvider * Change parent context logic * Use useLayoutEffect * Go back to syncing store in edit component * WIP: Move bindings logic to `getBlockAttributes` * WIP: Move bindings setAttributes logic to updateBlockBindings * Pass only `select` to `getValue` functions * Remove old editor hook * Add fallback to postId until context is ready * Remove setValue post-meta code * Simplify fallback conditional * Change bindings destructuring * Check canBindAttribute in updateBlockAttributes * Update unit tests to expect a dispatch * Add conditional in block * Don't use `getBlockAttributes` inside `getValue` * Revert "Don't use `getBlockAttributes` inside `getValue`" This reverts commit 0e91129. * Avoid processing bindings recursively * Access context through selector * Update getBlockAttributes logic * Don't use fallbacks * Add edit value posibility for post meta, add function to check if is admin * Revert "Add edit value posibility for post meta, add function to check if is admin" This reverts commit 9659455. * Test editing is allowed in paragraph block * Test protected fields are not editable * Revert "Enable parallel processing for PHPCS sniffs (WordPress#61700)" This reverts commit 8331820. * Add post meta setValue function * Update tests to check contenteditable * Update lockAttributesEditing default * Pass arguments to lockAttributesEditing * Check user can edit post meta * Check field is exposed in the REST API * Disable editing in templates * Add fallback for postId * Revert "Revert "Enable parallel processing for PHPCS sniffs (WordPress#61700)"" This reverts commit e74d71c. * Adapt old tests * Simplify lockAttributesEditing fallbacks * Don't use fallback for context * Add postType fallback when locking controls * Pass context in rich text * Check contenteditable attribute in test * Change name to `canUserEditValue` * Revert changes caused by rebasing * Add space back * Pass block context through rich text * Change imports * Transform block attributes into bindings in split selection * Pass context to in use-input * Change REST API check * Use getBoundAttributesValues * Don't split when attribute is bound * Revert changes caused by rebase * Cover more blocks when editing custom fields * Add a warning when pasting blocks --------- Co-authored-by: Carlos Bravo <carlos.bravo@automattic.com>
What?
Use the latest changes to the block bindings API in the editor to allow users to edit the value of the custom fields directly through the blocks when they are connected. For example, when the paragraph content is bound to a specific custom field, and the user start typing, they would be modifying the custom field and not the content.
At least in this first iteration, it should be limited to:
How?
Using the
setValue
function based on the code created by @ellatrix here. Apart from that, we should use thelockAttributesEditing
callback added in this pull request to cover the mentioned limitation.Additionally, it ensure that the split selection work as expected in bound blocks.
Testing Instructions
I added a couple of e2e tests for this use case. You can test it manually by:
Testing Instructions
1. Register post meta by adding this snippet to your theme's functions.php
2. Add a paragraph block bound to the custom field using the Code Editor