-
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 Controls SlotFill: refactor to allow passing multiple contexts, including internal components context #51264
Changes from 3 commits
86f704b
713c6db
17abb77
d3d28b2
98ad917
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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. 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. Do we also use the components context system in React Native code? Because I added the provider teleporting only to the web 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. Also it seems that in React Native we don't support the 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. 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?
If that's the case, though, then forwarding such contexts is not necessary, as you say. 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 pushed a commit that removes context forwarding (teleporting) from the We'll need to reintroduce the forwarding when we migrate mobile slotfills to the portal 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. Thanks for the call out @ciampo 🙇 ! As you shared in the above comments, the 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! |
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.
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 adocument
prop instead ofvalue
.It would be nice to remove the hard-coded
StyleProvider
from theFill
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 thefillProps
format.The
StyleProvider
s 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
emotion
library (and thereforeStyleProvider
) was always meant to be kept as an internal implementation detail to the package. Adopting your suggested approach would require exportingStyleProvider
from 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@wordpress/components
components would needStyleProvider
to render correctly, meaning that most probably every usage ofSlot
would need to passStyleProvider
StyleProvider
from theFill
implementation has the potential to cause visual breaking changes to third-party consumersThere 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.
What is
ComponentsContext
? CanToolbarContext
be part of it?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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, so
ToolbarContext
is basically the same thing asComponentsContext
but specific toToolbar
components. I wonder if there's a way to unify.