Try: Sync block context with store #61895
Closed
Conversation
This file contains 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
ellatrix
reviewed
May 23, 2024
| useDispatch( blockEditorStore ).updateBlockContext( | ||
| clientId, | ||
| blockContext | ||
| ); |
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
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
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?
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?
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.
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?
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.
There was a problem hiding this comment.
Do you mean something like this try?
|
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
|
SantosGuillamot
force-pushed
the
try/sync-block-context-in-store
branch
from
64984f2
to
c939061
Compare
ellatrix
reviewed
May 24, 2024
| @@ -29,6 +35,7 @@ export function BlockContextProvider( { value, children } ) { | |||
| () => ( { ...context, ...value } ), | |||
| [ context, value ] | |||
| ); | |||
| useDispatch( blockEditorStore ).updateBlockContext( nextValue ); | |||
SantosGuillamot
force-pushed
the
try/sync-block-context-in-store
branch
from
c939061
to
b1b768c
Compare
|
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. |
SantosGuillamot
requested review from
ajitbohra,
ryanwelcher,
juanmaguitar,
fabiankaegy and
ndiego
as code owners
|
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. |
SantosGuillamot
added
[Type] New API
|
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
|
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. |
|
Yes, let's close this one and revisit it if considered necessary in the future. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.