-
-
Notifications
You must be signed in to change notification settings - Fork 31.8k
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
[ExpansionPanel] Use context instead of cloneElement #18085
Conversation
@material-ui/core: parsed: +0.05% , gzip: +0.12% Details of bundle changes.Comparing: c28697d...69a6b92
|
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.
Code which forward rest props to the actual ExpansionPanel summary will still work the same.
The idea sounds good on paper. I expect very few people to rely on the hidden injected props (cloneElement) to customize the component. If people do, they should be able to recover by controlling the component.
Co-Authored-By: Tim Bormans <hello@timbormans.com>
@@ -5,7 +5,7 @@ import clsx from 'clsx'; | |||
import ButtonBase from '../ButtonBase'; | |||
import IconButton from '../IconButton'; | |||
import withStyles from '../styles/withStyles'; | |||
import ExpansionPanelContext from '../ExpansionPanel/Context'; | |||
import ExpansionPanelContext from '../ExpansionPanel/ExpansionPanelContext'; |
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.
The title of the commit might suggest that you prefer Context
, I do agree that it's unnecessarily long. I think that we should change all the components 👌.
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.
Nah it's fine for most of the tooling. In vscode you get the display of the file and dirname so it is information duplication. But as far as I know many people still have an issue with ExpansionPanel/index.js
because then they only see index.js in their IDE. As long as people still rely on the filename this is fine. But as always I recommend working these issues from both ends :)
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.
Alright, let's move to the next problem :).
Closes #15763, part of #12921
Prop removal isn't an issue since they were never document (
@ignore
). Code which forward rest props to the actual ExpansionPanel summary will still work the same.