[ExpansionPanel] Add useExpansionPanel hook #14616
Conversation
eps1lon
added
new feature
eps1lon
force-pushed
the
feat/ExpansionPanel/context
branch
from
1326b5f to
f14e12f
Compare
|
This PR introduced some changes to the bundle size. @material-ui/core: parsed: +0.05%, gzip: +0.19% Details of bundle changes.Comparing: f0cdca8...f14e12f
|
Should it be material-ui-size-bot (or just mui-size-bot to follow mu-bot naming)? Looks great though! |
| warning( | ||
| false, | ||
| [ | ||
| 'Material-UI: You called onChange from a component that is not a child of a ', |
There was a problem hiding this comment.
We could revisit this convention
| 'Material-UI: You called onChange from a component that is not a child of a ', | |
| 'Material-UI: you called onChange from a component that is not a child of a ', |
| if (!this.isControlled) { | ||
| // not controlled, use internal state | ||
| this.state.expanded = props.defaultExpanded !== undefined ? props.defaultExpanded : false; | ||
| function useIsControlled(value) { |
There was a problem hiding this comment.
What's the value of this abstraction?
There was a problem hiding this comment.
This was me playing around a bit. I have a more generic abstraction locally that will deal with the controlled vs. uncontrolled value in general.
Though even in this primitive state it adds value: We can move possible discussions about what constitutes a "controlled" value into a single place (see #14092 (comment)). This would ensure a consistent controlled vs. uncontrolled API.
| ].join('\n'), | ||
| ); | ||
|
|
||
| if (isMuiElement(child, ['ExpansionPanelSummary'])) { |
There was a problem hiding this comment.
How are you going to solve this problem? Though, I don't see the value of using the context if we still have to expose .muiName = 'ExpansionPanelSummary'.
There was a problem hiding this comment.
I was a bit premature with that statement. As soon as I looked at the code I realized we wouldn't gain as much as I initially thought.
What context would allow though is reaching into the ExpansionPanelSummary, grabbing the html ID, expose this in the context and then the content can get a aria-labelledby.
Context is definitely more powerful here. I need to finish up the implementation to fully understand what features this enables.
There was a problem hiding this comment.
I'm happy that you are mentioning a11y. So far, we have had a hard time abstracting the accessibility label logic. It often requires the introducing of a global index structure or the usage of the context to propagate the changes down the tree. Our tradeoff so far has always been to defer the logic to the developers, saving, what we thought bundle size.
There was a problem hiding this comment.
Context is currently my preferred solution to these problems. I like neither the SomePassedComponent prop + SomePassedComponentProps prop pattern nor cloneElement. It feels like black magic from the outside.
The Context + useSomeExposedPartFromTheContext custom hook pattern is way more predictable. While it might be more bundle size it resembles more what we're actually trying to achieve. The previous patterns only tried to emulate some context. Now we actually have some. Without the downsides of possibly defeating memoized components or props collision.
And with the new react-devtools debugging should be easier i.e. you actually see where the props are coming from.
eps1lon
force-pushed
the
feat/ExpansionPanel/context
branch
from
f14e12f to
ab39b2f
Compare
eps1lon
force-pushed
the
feat/ExpansionPanel/context
branch
from
ab39b2f to
604b37c
Compare
Switching from cloneElement to Context. This is not ready. Just want to take material-size-bot for a ride.