Block Controls SlotFill: refactor to allow passing multiple contexts, including internal components context #51264
Conversation
…nts context (cherry picked from commit d7d169783b89b0838f2add86852b9cb378fdc890)
ciampo
added
[Type] Enhancement
|
@jsnajdr — my only question at this point is: should the components context be passed specifically in every instance of |
There was a problem hiding this comment.
Do we also use the components context system in React Native code? Because I added the provider teleporting only to the web BlockControlsSlot, not to the mobile one.
There was a problem hiding this comment.
Also it seems that in React Native we don't support the bubblesVirtually variant with portals. The bubblesVirtually prop is simply ignored. The Fill is always rendered in the Slot's React tree. It doesn't see context from the Fill tree, and teleporting the toolbar context is not needed, the component already sees it.
There was a problem hiding this comment.
Folks from the mobile team may be best suited to answer here.
From a quick look, I don't think that there are native components explicitly using the components context system, although it is my understanding that web versions of components could be used by React Native, meaning that we'll likely neet to pass all the context values that we do in the web counterpart?
Also it seems that in React Native we don't support the bubblesVirtually variant with portals. The bubblesVirtually prop is simply ignored. The Fill is always rendered in the Slot's React tree. It doesn't see context from the Fill tree, and teleporting the toolbar context is not needed, the component already sees it.
If that's the case, though, then forwarding such contexts is not necessary, as you say.
There was a problem hiding this comment.
I pushed a commit that removes context forwarding (teleporting) from the slot.native.js component completely, including the pre-exisiting ToolbarContext. Mobile uses the non-virtual Slot implementation, and all these contexts (toolbar, component system, Emotion styles) are already present it its parent React tree.
We'll need to reintroduce the forwarding when we migrate mobile slotfills to the portal bubblesVirtually implementation.
There was a problem hiding this comment.
Thanks for the call out @ciampo 🙇 ! As you shared in the above comments, the SlotFill implementation in React Native is limited compared to the web version as we don't support bubblesVirtually. I checked the usage of ToolbarContext and seems we are not using it, only in the web version, so I agree on removing it from here.
Web:
Native:
Regarding these changes, I tested locally and I encountered an error that I shared here. Let me know if I can help anyhow, thanks!
| const fillProps = useMemo( | ||
| () => [ | ||
| [ ToolbarContext.Provider, { value: toolbarState } ], | ||
| [ ComponentsContext.Provider, { value: contextState } ], |
There was a problem hiding this comment.
Here I did a little change, passing a props object instead of a value for a hard-coded value prop. That makes the system more flexible.
We'll want to migrate the <StyleProvider document={...}> usage also to this format, and there's a document prop instead of value.
It would be nice to remove the hard-coded StyleProvider from the Fill implementation, as added in #38237, and configure the slots and fills that really need this explicitly.
@youknowriad added StyleProvider wrappers around specific fills in #31073. There should be eventually migrated to the fillProps format.
The StyleProviders from #31073 (specific places) and #38237 (universal for every <Fill bubblesVirtually>) often overlap each other, rendering the same provider two times next to each other. I believe the universal one should eventually go away.
There was a problem hiding this comment.
Thank you for the extra context, Jarda! Your suggestion makes sense in terms of code hygiene and separation of concerns, although there are a few aspects that we should also take into account:
- the
emotionlibrary (and thereforeStyleProvider) was always meant to be kept as an internal implementation detail to the package. Adopting your suggested approach would require exportingStyleProviderfrom the package. Although maybe we can still keep it as a "generic" style provider (not necessarily Emotion-themed), and if one day we migrate away from Emotion that the provider is not needed anymore, we can leave it around as an empty, no-op context - a good portion of
@wordpress/componentscomponents would needStyleProviderto render correctly, meaning that most probably every usage ofSlotwould need to passStyleProvider - related to the point above, removing
StyleProviderfrom theFillimplementation has the potential to cause visual breaking changes to third-party consumers
There was a problem hiding this comment.
What is ComponentsContext? Can ToolbarContext be part of it?
There was a problem hiding this comment.
ComponentsContext is a kind of a generic key-value store, where a parent component can tell child components things like "inside toolbar dropdown menus should be in this toolbar-specific style" or "inside input control the prefix subcomponent should have a certain padding-left". Originally comes from Q and his G2 system.
ToolbarContext is a thing coming from Reakit, where a parent component (container) exposes some API for children (items).
There was a problem hiding this comment.
Yeah, so ToolbarContext is basically the same thing as ComponentsContext but specific to Toolbar components. I wonder if there's a way to unify.
|
Size Change: -3.07 kB (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
Yes, answered it here: #51264 (comment). The
|
|
Flaky tests detected in 98ad917. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/5198427281
|
|
One more item that needs addressing (quoting from #51154 (comment)):
|
… is present anyway
Maybe forwarding the components context from the Slot tree, without merging it with the context from the Fill tree, is actually the right thing to do. Typically the |
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
I believe this can be merged now, if all tests pass. CI checks complain about a missing changelog entry for @wordpress/components, but we're adding only a new private API, nothing publicly visible.
… including internal components context (WordPress#51264) * SlotFill: pass array of contexts in fillProps, use it to pass components context (cherry picked from commit d7d169783b89b0838f2add86852b9cb378fdc890) * Use lock/unlock instead of exporting experimental API * Pass props object instead of just value prop * Mobile BlockControlsSlot: remove ToolbarContext teleport, the context is present anyway * Put forwardedContext into a fillProps field --------- Co-authored-by: Jarda Snajdr <[email protected]>
What?
While working on #51154, with the help of @jsnajdr we realised that components rendering in the block toolbar via the block controls
SlotFillwouldn't have access to the internal components context.This PR extracts @jsnajdr 's proposed solution, so that it can be reviewed and merged separately from the rest of the changes in that PR.
Why?
In order to function as expected, components from
@wordpress/componentsneed to access data passed via React Context. The issue is that, when usingSlotFill, the components rendered in theFilldon't normally have access to the context in which theFillrenders.How?
The solution is to manually forward these contexts in the
SlotFillcomponent. This PR refactors the code taking care of context forwarding, making it more general and thus allowing for an additional context (the internal components' context) to also be forwarded.The internal components context is a private API, and is made available to other packages via
lock/unlock.Testing Instructions
This PR is not expected to cause any differences in how the block editor works — it is just an internal change that will allow more components to work seamlessly.
Play around with the block toolbar in the block editor, everything should keep working as on
trunk.