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.

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

Merged
merged 4 commits into from
Oct 31, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
33 changes: 20 additions & 13 deletions packages/material-ui/src/ExpansionPanel/ExpansionPanel.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import { chainPropTypes } from '@material-ui/utils';
import Collapse from '../Collapse';
import Paper from '../Paper';
import withStyles from '../styles/withStyles';
import ExpansionPanelContext from './ExpansionPanelContext';

export const styles = theme => {
const transition = {
Expand Down Expand Up @@ -115,17 +116,25 @@ const ExpansionPanel = React.forwardRef(function ExpansionPanel(props, ref) {
}, [expandedProp, isControlled]);
}

const handleChange = event => {
if (!isControlled) {
setExpandedState(!expanded);
}
const handleChange = React.useCallback(
event => {
if (!isControlled) {
setExpandedState(!expanded);
}

if (onChange) {
onChange(event, !expanded);
}
};
if (onChange) {
onChange(event, !expanded);
}
},
[expanded, isControlled, onChange],
);

const [summary, ...children] = React.Children.toArray(childrenProp);
const contextValue = React.useMemo(() => ({ expanded, disabled, toggle: handleChange }), [
expanded,
disabled,
handleChange,
]);

return (
<Paper
Expand All @@ -142,11 +151,9 @@ const ExpansionPanel = React.forwardRef(function ExpansionPanel(props, ref) {
square={square}
{...other}
>
{React.cloneElement(summary, {
disabled,
expanded,
onChange: handleChange,
})}
<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
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
import React from 'react';

/**
* @ignore - internal component.
* @type {React.Context<{} | {expanded: boolean, disabled: boolean, toggle: () => void}>}
*/
const ExpansionPanelContext = React.createContext({});

export default ExpansionPanelContext;
Original file line number Diff line number Diff line change
Expand Up @@ -8,11 +8,8 @@ export type ExpansionPanelSummaryTypeMap<
D extends React.ElementType = 'div'
> = ExtendButtonBaseTypeMap<{
props: P & {
disabled?: boolean;
expanded?: boolean;
expandIcon?: React.ReactNode;
IconButtonProps?: Partial<IconButtonProps>;
onChange?: React.ReactEventHandler<{}>;
};
defaultComponent: D;
classKey: ExpansionPanelSummaryClassKey;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import clsx from 'clsx';
import ButtonBase from '../ButtonBase';
import IconButton from '../IconButton';
import withStyles from '../styles/withStyles';
import ExpansionPanelContext from '../ExpansionPanel/ExpansionPanelContext';
Copy link
Member

@oliviertassinari oliviertassinari Oct 31, 2019

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 Expand Up @@ -68,12 +69,9 @@ const ExpansionPanelSummary = React.forwardRef(function ExpansionPanelSummary(pr
children,
classes,
className,
disabled = false,
expanded,
expandIcon,
IconButtonProps,
onBlur,
onChange,
onClick,
onFocusVisible,
...other
Expand All @@ -94,10 +92,11 @@ const ExpansionPanelSummary = React.forwardRef(function ExpansionPanelSummary(pr
onBlur(event);
}
};
// TODO: Remove in v5 and use onClick in ExpansionPanel.js

const { disabled = false, expanded, toggle } = React.useContext(ExpansionPanelContext);
const handleChange = event => {
if (onChange) {
onChange(event);
if (toggle) {
toggle(event);
}
if (onClick) {
onClick(event);
Expand Down Expand Up @@ -160,16 +159,6 @@ ExpansionPanelSummary.propTypes = {
* @ignore
*/
className: PropTypes.string,
/**
* @ignore
* If `true`, the summary will be displayed in a disabled state.
*/
disabled: PropTypes.bool,
/**
* @ignore
* If `true`, expands the summary, otherwise collapse it.
*/
expanded: PropTypes.bool,
/**
* The icon to display as the expand indicator.
*/
Expand All @@ -182,10 +171,6 @@ ExpansionPanelSummary.propTypes = {
* @ignore
*/
onBlur: PropTypes.func,
/**
* @ignore
*/
onChange: PropTypes.func,
/**
* @ignore
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import { spy } from 'sinon';
import { createMount, getClasses } from '@material-ui/core/test-utils';
import describeConformance from '../test-utils/describeConformance';
import { createClientRender, fireEvent } from 'test/utils/createClientRender';
import ExpansionPanel from '../ExpansionPanel';
import ExpansionPanelSummary from './ExpansionPanelSummary';
import ButtonBase from '../ButtonBase';

Expand All @@ -13,6 +14,7 @@ describe('<ExpansionPanelSummary />', () => {
const render = createClientRender({ strict: true });

before(() => {
// requires mocking the TransitionComponent of `ExpansionPanel`
mount = createMount({ strict: true });
classes = getClasses(<ExpansionPanelSummary />);
});
Expand All @@ -33,13 +35,21 @@ describe('<ExpansionPanelSummary />', () => {
});

it('when disabled should have disabled class', () => {
const { getByRole } = render(<ExpansionPanelSummary disabled />);
const { getByRole } = render(
<ExpansionPanel disabled TransitionComponent={({ children }) => children}>
<ExpansionPanelSummary />
</ExpansionPanel>,
);

expect(getByRole('button')).to.have.class(classes.disabled);
});

it('when expanded adds the expanded class to the button and expandIcon', () => {
const { container, getByRole } = render(<ExpansionPanelSummary expanded expandIcon="expand" />);
const { container, getByRole } = render(
<ExpansionPanel expanded TransitionComponent={({ children }) => children}>
<ExpansionPanelSummary expandIcon="expand" />
</ExpansionPanel>,
);

const button = getByRole('button');
expect(button).to.have.class(classes.expanded);
Expand Down Expand Up @@ -93,9 +103,13 @@ describe('<ExpansionPanelSummary />', () => {
expect(handleClick.callCount).to.equal(1);
});

it('calls onChange when clicking', () => {
it('fires onChange of the ExpansionPanel if clicked', () => {
const handleChange = spy();
const { getByRole } = render(<ExpansionPanelSummary onChange={handleChange} />);
const { getByRole } = render(
<ExpansionPanel onChange={handleChange} TransitionComponent={({ children }) => children}>
<ExpansionPanelSummary />
</ExpansionPanel>,
);

getByRole('button').click();

Expand Down