Try: Sync block context with store#61895
Closed
SantosGuillamot wants to merge 10 commits intotrunkfrom
Closed
Conversation
ellatrix
reviewed
May 23, 2024
| useDispatch( blockEditorStore ).updateBlockContext( | ||
| clientId, | ||
| blockContext | ||
| ); |
Member
There was a problem hiding this comment.
We can't call actions on render, must be used to an effect, see https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/inner-blocks/use-nested-settings-update.js
Member
There was a problem hiding this comment.
Also, we shouldn't do this for every block, we should only do if where context it provided
Member
There was a problem hiding this comment.
Actually, looking at https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/inner-blocks/use-block-context.js, it appears block context is just a selector? So we could derive the whole thing the getBlockAttributes selector?
Member
There was a problem hiding this comment.
So in getBlockAttributes, we should loop up the parents, see if they provide context, and if so set their attributes as context?
Contributor
Author
There was a problem hiding this comment.
Good points 🙂
I thought we needed to get the value from useContext( BlockContext ), as you were doing here, but maybe you are right and we can do that looping up the parents. I can give that a try.
Contributor
Author
There was a problem hiding this comment.
That wouldn't include context provided by BlockContextProvider that doesn't belong to a block, right?
I'm thinking that, if I am not mistaken, we are providing postId and postType as context in the post template: link. But that wouldn't be available iterating through the parents, right?
Member
There was a problem hiding this comment.
Hm, I guess then it is easier to sync the context. I'd sync it inside of BlockContextProvider. Then create a selector that gets the context which would loop through the parents and finds/merges the context.
|
Size Change: -5.49 kB (-0.31%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 64984f2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9205619719
|
64984f2 to
c939061
Compare
ellatrix
reviewed
May 24, 2024
| () => ( { ...context, ...value } ), | ||
| [ context, value ] | ||
| ); | ||
| useDispatch( blockEditorStore ).updateBlockContext( nextValue ); |
c939061 to
b1b768c
Compare
Contributor
Author
|
I've been taking a deeper look at this and I am not sure how the context syncing should work. Let me use the query loop & post template example to explain my concerns. When we have a query loop with a post template, the post template provides the If I am not mistaken, the Maybe we should rely on EDIT: I went back to the previous implementation of syncing the store in the edit component and it seems to work better. |
Contributor
Author
|
I've been testing this pull request, plus one to move the bindings logic and to allow post meta editing, and everything seems to work as expected. I'm marking this pull request as ready for review, but there are some outstanding questions:
|
|
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. |
Contributor
Author
|
As it seems it is still not clear if we should expose the block context in the store, I've moved the action and selector to private because it seems safer to me. |
29 tasks
64 tasks
Member
|
It looks like we won't need it after all. Based on the progress on other tasks planned for WordPress 6.7 outlined in #63018, I suggest we close this PR. |
Contributor
Author
|
Yes, let's close this one and revisit it if considered necessary in the future. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
The idea behind this pull request is to sync the block context in the store.
Why?
To be able to access it without using a hook for uses cases like bindings modifying the block attributes.
How?
Creating a new action
updateBlockContextto modify the block context in the store and a selectorgetBlockContextto retrieve it.Apart from that, I'm syncing the context in the edit component using
useContext.Testing Instructions
getBlockContextselector and check that the block context is present.