Components: Add component system context
#30877
Merged
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
sarayourfriend
added
[Package] Components
sarayourfriend
requested review from
ajitbohra and
chrisvanpatten
as code owners
|
Size Change: +922 B (0%) Total Size: 1.47 MB
ℹ️ View Unchanged
|
gziolo
reviewed
Apr 16, 2021
gziolo
reviewed
Apr 16, 2021
This has limited usage because you can only pass down data/settings through the context for components that the parent component defines. Unless the consuming component will call |
|
I'm a little confused @gziolo. Maybe there is a misunderstanding of how this works, but the receiving component just has to declare it's name in the Maybe I'm misunderstanding what you're saying though? |
|
I guess the confusion comes from the fact that you can set the context with the I guess the idea is to use it the way it's done with Storybook stories, where the context is overridden outside of the actual components. This way you can tweak child components to have different props without using prop drilling anti-pattern. My biggest concern is why it's all based on the convention where you provide the name of the components and the key-value pairs for props. In practice, it creates two issues that I'm not sure if they are easy to solve:
|
You can't, that is a downside to this powerful and only rarely-necessary tool. Perhaps we can just restrict the API of a ButtonGroup to only allow the following: <ButtonGroup>
<Button />
<Button />
</ButtonGroup>And then follow the regular pattern of passing props via |
sarayourfriend
force-pushed
the
add/context
branch
from
28c4bda to
e57e799
Compare
sarayourfriend
requested review from
adamsilverstein,
adamziel,
ajlende,
atimmer,
cameronvoell,
dmsnell,
draganescu,
ellatrix,
guarani,
kevin940726,
mkaz,
mmtr,
nerrad and
noahtallen
as code owners
sarayourfriend
removed request for
cameronvoell,
chrisvanpatten,
mmtr,
nerrad,
guarani,
adamsilverstein,
TimothyBJacobs,
ellatrix,
ajlende,
dmsnell,
noahtallen,
kevin940726,
tellthemachines and
mkevins
|
Sorry for the pings everybody, I had the wrong base branch on this PR and it requested a ton of people as codeowners due to trunk changes! Sorry!!!!!!! |
youknowriad
reviewed
Apr 21, 2021
youknowriad
reviewed
Apr 21, 2021
|
@youknowriad @gziolo can I get a re-review on this? Thanks 🙂 After this PR we can start converting existing usages of G2 to use vanilla emotion. |
gziolo
reviewed
Apr 22, 2021
| @@ -1,2 +1,8 @@ | |||
| export { ComponentSystemProvider } from './component-system-provider'; | |||
| export { withNext } from './with-next'; | |||
| export { | |||
There was a problem hiding this comment.
New methods aren't exposed outside or @wordpress/components for now, right?
There was a problem hiding this comment.
Nope! None of these are exposed in components/src/index.js as that file explicitly only exports ComponentsSystemProvider and withNext, both as experimental anyway.
gziolo
approved these changes
Apr 22, 2021
|
Is there any documentation explaining how it all works? 😄 There are stories that are fine for start but we should be actively thinking how to make those complex APIs approachable for usage outside of Gutenberg. |
|
I think this one is breaking the build. I just did a fresh update and run |
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.
Description
Adds the component system
context.The component system's
contextis a pretty powerful tool. It allows us to safely pass props to children regardless of how far down the component tree they are. For example, if you have aButtonGroupcomponent and aButtoncomponent, you may want to pass some props to theButtonto get it to render correctly (for example, to get them to render inline with each other).In that case you would create your
ButtonGroupcomponent the following way:Et viola! Now ever
Buttonrendered inside ofButtonGroupregardless of where in the component tree it lives will receive theinline: trueprop (so long as theButtonalso callsuseContextSystem( props, 'Button' )which connects it to the context.Depends on #30630
How has this been tested?
Unit tests and storybook.
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).