Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ExpansionPanel] Use context instead of cloneElement #18085

Merged
merged 4 commits into from
Oct 31, 2019
Merged

[ExpansionPanel] Use context instead of cloneElement #18085

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
58f3d6f
[ExpansionPanel] Use context instead of cloneElement
eps1lon Oct 29, 2019
2c8d5a0
fix: "no suitable component definition found"
eps1lon Oct 29, 2019
373f6dd
Apply suggestions from code review
eps1lon Oct 31, 2019
69a6b92
More information duplication
eps1lon Oct 31, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions packages/material-ui/src/ExpansionPanel/ExpansionPanel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import { chainPropTypes } from '@material-ui/utils';
import Collapse from '../Collapse';
import Paper from '../Paper';
import withStyles from '../styles/withStyles';
import Context from './Context';
import ExpansionPanelContext from './ExpansionPanelContext';

export const styles = theme => {
const transition = {
Expand Down Expand Up @@ -151,7 +151,9 @@ const ExpansionPanel = React.forwardRef(function ExpansionPanel(props, ref) {
square={square}
{...other}
>
<Context.Provider value={contextValue}>{summary}</Context.Provider>
<ExpansionPanelContext.Provider value={contextValue}>
{summary}
</ExpansionPanelContext.Provider>
<TransitionComponent in={expanded} timeout="auto" {...TransitionProps}>
<div aria-labelledby={summary.props.id} id={summary.props['aria-controls']} role="region">
{children}
Expand Down
0 ...material-ui/src/ExpansionPanel/Context.js → ...c/ExpansionPanel/ExpansionPanelContext.js
View file
Open in desktop
File renamed without changes.
2 changes: 1 addition & 1 deletion packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Copy link
Member

@oliviertassinari oliviertassinari Oct 31, 2019 •
edited
Loading

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 👌.

Copy link
Member Author

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 :)

Copy link
Member

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 :).


export const styles = theme => {
const transition = {
Expand Down