From 552024b98fd1ac24395d7322d957598ec33a8f10 Mon Sep 17 00:00:00 2001 From: ayliao Date: Sun, 20 Oct 2019 23:34:07 -0700 Subject: [PATCH 1/3] [ExpansionPanel] Added demo for actions in ExpansionPanelSummary (#9427). --- .../ActionsInExpansionPanelSummary.js | 94 ++++++++++++++++++ .../ActionsInExpansionPanelSummary.tsx | 96 +++++++++++++++++++ .../expansion-panels/expansion-panels.md | 10 ++ 3 files changed, 200 insertions(+) create mode 100644 docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.js create mode 100644 docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.tsx diff --git a/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.js b/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.js new file mode 100644 index 00000000000000..067263034f7929 --- /dev/null +++ b/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.js @@ -0,0 +1,94 @@ +import React from 'react'; +import { makeStyles } from '@material-ui/core/styles'; +import ExpansionPanel from '@material-ui/core/ExpansionPanel'; +import ExpansionPanelSummary from '@material-ui/core/ExpansionPanelSummary'; +import ExpansionPanelDetails from '@material-ui/core/ExpansionPanelDetails'; +import Checkbox from '@material-ui/core/Checkbox'; +import FormControlLabel from '@material-ui/core/FormControlLabel'; +import Typography from '@material-ui/core/Typography'; +import ExpandMoreIcon from '@material-ui/icons/ExpandMore'; + +const useStyles = makeStyles(theme => ({ + root: { + width: '100%', + }, + details: { + fontSize: theme.typography.pxToRem(15), + color: theme.palette.text.secondary, + }, +})); + +export default function ActionsInExpansionPanelSummary() { + const classes = useStyles(); + + return ( +
+ + } + aria-label="Expand" + aria-controls="panel1a-content" + id="panel1a-header" + > + event.stopPropagation()} + onFocus={event => event.stopPropagation()} + control={} + label="I acknowledge that I should stop the click event propagation" + /> + + + + The click event of the nested action will propagate up and expand the panel unless you + explicitly stop it. + + + + + } + aria-label="Expand" + aria-controls="panel2a-content" + id="panel2a-header" + > + event.stopPropagation()} + onFocus={event => event.stopPropagation()} + control={} + label="I acknowledge that I should stop the focus event propagation" + /> + + + + The focus event of the nested action will propagate up and also focus the expansion + panel unless you explicitly stop it. + + + + + } + aria-label="Expand" + aria-controls="panel3a-content" + id="panel3a-header" + > + event.stopPropagation()} + onFocus={event => event.stopPropagation()} + control={} + label="I acknowledge that I should provide an aria-label on each action that I add" + /> + + + + If you forget to put an aria-label on the nested action, the label of the action will + also be included in the label of the parent button that controls the panel expansion. + + + +
+ ); +} diff --git a/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.tsx b/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.tsx new file mode 100644 index 00000000000000..ecc1f1eb3f9175 --- /dev/null +++ b/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.tsx @@ -0,0 +1,96 @@ +import React from 'react'; +import { Theme, createStyles, makeStyles } from '@material-ui/core/styles'; +import ExpansionPanel from '@material-ui/core/ExpansionPanel'; +import ExpansionPanelSummary from '@material-ui/core/ExpansionPanelSummary'; +import ExpansionPanelDetails from '@material-ui/core/ExpansionPanelDetails'; +import Checkbox from '@material-ui/core/Checkbox'; +import FormControlLabel from '@material-ui/core/FormControlLabel'; +import Typography from '@material-ui/core/Typography'; +import ExpandMoreIcon from '@material-ui/icons/ExpandMore'; + +const useStyles = makeStyles((theme: Theme) => + createStyles({ + root: { + width: '100%', + }, + details: { + fontSize: theme.typography.pxToRem(15), + color: theme.palette.text.secondary, + }, + }), +); + +export default function ActionsInExpansionPanelSummary() { + const classes = useStyles(); + + return ( +
+ + } + aria-label="Expand" + aria-controls="panel1a-content" + id="panel1a-header" + > + event.stopPropagation()} + onFocus={event => event.stopPropagation()} + control={} + label="I acknowledge that I should stop the click event propagation" + /> + + + + The click event of the nested action will propagate up and expand the panel unless you + explicitly stop it. + + + + + } + aria-label="Expand" + aria-controls="panel2a-content" + id="panel2a-header" + > + event.stopPropagation()} + onFocus={event => event.stopPropagation()} + control={} + label="I acknowledge that I should stop the focus event propagation" + /> + + + + The focus event of the nested action will propagate up and also focus the expansion + panel unless you explicitly stop it. + + + + + } + aria-label="Expand" + aria-controls="panel3a-content" + id="panel3a-header" + > + event.stopPropagation()} + onFocus={event => event.stopPropagation()} + control={} + label="I acknowledge that I should provide an aria-label on each action that I add" + /> + + + + If you forget to put an aria-label on the nested action, the label of the action will + also be included in the label of the parent button that controls the panel expansion. + + + +
+ ); +} diff --git a/docs/src/pages/components/expansion-panels/expansion-panels.md b/docs/src/pages/components/expansion-panels/expansion-panels.md index f4446e19975a16..04fc1c2440cdef 100644 --- a/docs/src/pages/components/expansion-panels/expansion-panels.md +++ b/docs/src/pages/components/expansion-panels/expansion-panels.md @@ -28,6 +28,16 @@ Here is an example of customizing the component. You can learn more about this i {{"demo": "pages/components/expansion-panels/CustomizedExpansionPanels.js"}} +## Adding actions inside the expansion panel summary + +If you would like to put an action such as a `Checkbox` or a `Button` inside of the `ExpansionPanelSummary`, you will +need to remember to stop the propagation of the focus and click events of the action in order to prevent the panel from +expanding/collapsing when using the action. +You should also provide an `aria-label` for the action, otherwise the label of the nested action will be included in +the label of the parent button that controls the panel expansion. + +{{"demo": "pages/components/expansion-panels/ActionsInExpansionPanelSummary.js"}} + ## Performance The content of ExpansionPanels is mounted by default even if the panel is not expanded. From 46668ae92caa945d513aa644ed9d79b253636a17 Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Mon, 21 Oct 2019 23:13:10 +0200 Subject: [PATCH 2/3] Apply feedback --- .../ActionsInExpansionPanelSummary.js | 26 ++++++-------- .../ActionsInExpansionPanelSummary.tsx | 36 ++++++++----------- .../DetailedExpansionPanel.js | 2 +- .../DetailedExpansionPanel.tsx | 2 +- .../expansion-panels/expansion-panels.md | 5 ++- .../ExpansionPanelSummary.js | 5 +-- .../ExpansionPanelSummary.test.js | 20 +++++------ 7 files changed, 42 insertions(+), 54 deletions(-) diff --git a/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.js b/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.js index 067263034f7929..821f8b8c126287 100644 --- a/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.js +++ b/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.js @@ -8,15 +8,11 @@ import FormControlLabel from '@material-ui/core/FormControlLabel'; import Typography from '@material-ui/core/Typography'; import ExpandMoreIcon from '@material-ui/icons/ExpandMore'; -const useStyles = makeStyles(theme => ({ +const useStyles = makeStyles({ root: { width: '100%', }, - details: { - fontSize: theme.typography.pxToRem(15), - color: theme.palette.text.secondary, - }, -})); +}); export default function ActionsInExpansionPanelSummary() { const classes = useStyles(); @@ -27,8 +23,8 @@ export default function ActionsInExpansionPanelSummary() { } aria-label="Expand" - aria-controls="panel1a-content" - id="panel1a-header" + aria-controls="additional-actions1-content" + id="additional-actions1-header" > - + The click event of the nested action will propagate up and expand the panel unless you explicitly stop it. @@ -49,8 +45,8 @@ export default function ActionsInExpansionPanelSummary() { } aria-label="Expand" - aria-controls="panel2a-content" - id="panel2a-header" + aria-controls="additional-actions2-content" + id="additional-actions2-header" > - + The focus event of the nested action will propagate up and also focus the expansion panel unless you explicitly stop it. @@ -71,8 +67,8 @@ export default function ActionsInExpansionPanelSummary() { } aria-label="Expand" - aria-controls="panel3a-content" - id="panel3a-header" + aria-controls="additional-actions3-content" + id="additional-actions3-header" > - + If you forget to put an aria-label on the nested action, the label of the action will also be included in the label of the parent button that controls the panel expansion. diff --git a/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.tsx b/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.tsx index ecc1f1eb3f9175..821f8b8c126287 100644 --- a/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.tsx +++ b/docs/src/pages/components/expansion-panels/ActionsInExpansionPanelSummary.tsx @@ -1,5 +1,5 @@ import React from 'react'; -import { Theme, createStyles, makeStyles } from '@material-ui/core/styles'; +import { makeStyles } from '@material-ui/core/styles'; import ExpansionPanel from '@material-ui/core/ExpansionPanel'; import ExpansionPanelSummary from '@material-ui/core/ExpansionPanelSummary'; import ExpansionPanelDetails from '@material-ui/core/ExpansionPanelDetails'; @@ -8,17 +8,11 @@ import FormControlLabel from '@material-ui/core/FormControlLabel'; import Typography from '@material-ui/core/Typography'; import ExpandMoreIcon from '@material-ui/icons/ExpandMore'; -const useStyles = makeStyles((theme: Theme) => - createStyles({ - root: { - width: '100%', - }, - details: { - fontSize: theme.typography.pxToRem(15), - color: theme.palette.text.secondary, - }, - }), -); +const useStyles = makeStyles({ + root: { + width: '100%', + }, +}); export default function ActionsInExpansionPanelSummary() { const classes = useStyles(); @@ -29,8 +23,8 @@ export default function ActionsInExpansionPanelSummary() { } aria-label="Expand" - aria-controls="panel1a-content" - id="panel1a-header" + aria-controls="additional-actions1-content" + id="additional-actions1-header" > - + The click event of the nested action will propagate up and expand the panel unless you explicitly stop it. @@ -51,8 +45,8 @@ export default function ActionsInExpansionPanelSummary() { } aria-label="Expand" - aria-controls="panel2a-content" - id="panel2a-header" + aria-controls="additional-actions2-content" + id="additional-actions2-header" > - + The focus event of the nested action will propagate up and also focus the expansion panel unless you explicitly stop it. @@ -73,8 +67,8 @@ export default function ActionsInExpansionPanelSummary() { } aria-label="Expand" - aria-controls="panel3a-content" - id="panel3a-header" + aria-controls="additional-actions3-content" + id="additional-actions3-header" > - + If you forget to put an aria-label on the nested action, the label of the action will also be included in the label of the parent button that controls the panel expansion. diff --git a/docs/src/pages/components/expansion-panels/DetailedExpansionPanel.js b/docs/src/pages/components/expansion-panels/DetailedExpansionPanel.js index 4fe1071686225c..21e1709fa6f1c6 100644 --- a/docs/src/pages/components/expansion-panels/DetailedExpansionPanel.js +++ b/docs/src/pages/components/expansion-panels/DetailedExpansionPanel.js @@ -73,7 +73,7 @@ export default function DetailedExpansionPanel() { Select your destination of choice
- + Learn more diff --git a/docs/src/pages/components/expansion-panels/DetailedExpansionPanel.tsx b/docs/src/pages/components/expansion-panels/DetailedExpansionPanel.tsx index 8f6b7ff870c67b..12d73dbdafd5f5 100644 --- a/docs/src/pages/components/expansion-panels/DetailedExpansionPanel.tsx +++ b/docs/src/pages/components/expansion-panels/DetailedExpansionPanel.tsx @@ -75,7 +75,7 @@ export default function DetailedExpansionPanel() { Select your destination of choice
- + Learn more diff --git a/docs/src/pages/components/expansion-panels/expansion-panels.md b/docs/src/pages/components/expansion-panels/expansion-panels.md index 04fc1c2440cdef..ca84c269dcbf4c 100644 --- a/docs/src/pages/components/expansion-panels/expansion-panels.md +++ b/docs/src/pages/components/expansion-panels/expansion-panels.md @@ -28,10 +28,9 @@ Here is an example of customizing the component. You can learn more about this i {{"demo": "pages/components/expansion-panels/CustomizedExpansionPanels.js"}} -## Adding actions inside the expansion panel summary +## Additional actions -If you would like to put an action such as a `Checkbox` or a `Button` inside of the `ExpansionPanelSummary`, you will -need to remember to stop the propagation of the focus and click events of the action in order to prevent the panel from +In order to put an action such as a `Checkbox` or a button inside of the `ExpansionPanelSummary`, you need to stop the propagation of the focus and click events to prevent the panel from expanding/collapsing when using the action. You should also provide an `aria-label` for the action, otherwise the label of the nested action will be included in the label of the parent button that controls the panel expansion. diff --git a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js index 81d71a8ff81b4c..5d3227e2db020a 100644 --- a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js +++ b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.js @@ -1,3 +1,4 @@ +/* eslint-disable jsx-a11y/aria-role */ import React from 'react'; import PropTypes from 'prop-types'; import clsx from 'clsx'; @@ -128,13 +129,13 @@ const ExpansionPanelSummary = React.forwardRef(function ExpansionPanelSummary(pr
{children}
{expandIcon && ( diff --git a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.test.js b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.test.js index 73c129ef216b3a..be6ef02b790263 100644 --- a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.test.js +++ b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.test.js @@ -39,21 +39,19 @@ describe('', () => { }); it('when expanded adds the expanded class to any button regardless of a11y', () => { - const { getAllByRole } = render(); - - const buttons = getAllByRole('button', { hidden: true }); - expect(buttons).to.have.length(2); - expect(buttons[0]).to.have.class(classes.expanded); - expect(buttons[0]).to.have.attribute('aria-expanded', 'true'); - expect(buttons[0]).not.to.be.inaccessible; - expect(buttons[1]).to.have.class(classes.expanded); - expect(buttons[1]).to.be.inaccessible; + const { container } = render(); + + expect(container.firstChild).to.have.class(classes.expanded); + expect(container.firstChild).to.have.attribute('aria-expanded', 'true'); + expect(container.firstChild).not.to.be.inaccessible; + expect(container.querySelector(`.${classes.expandIcon}`)).to.have.class(classes.expanded); + expect(container.querySelector(`.${classes.expandIcon}`)).to.be.inaccessible; }); it('should render with the expand icon and have the expandIcon class', () => { - const { getAllByRole } = render(Icon} />); + const { container } = render(Icon} />); - const expandButton = getAllByRole('button', { hidden: true })[1]; + const expandButton = container.querySelector(`.${classes.expandIcon}`); expect(expandButton).to.have.class(classes.expandIcon); expect(expandButton).to.have.text('Icon'); expect(expandButton).to.be.inaccessible; From bd1b98923281cf48ee2e471cdfa4ba227a5a8b2f Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Tue, 22 Oct 2019 10:39:08 +0200 Subject: [PATCH 3/3] Document new behavior --- .../ExpansionPanelSummary.test.js | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.test.js b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.test.js index be6ef02b790263..ce91479de99bad 100644 --- a/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.test.js +++ b/packages/material-ui/src/ExpansionPanelSummary/ExpansionPanelSummary.test.js @@ -38,23 +38,21 @@ describe('', () => { expect(getByRole('button')).to.have.class(classes.disabled); }); - it('when expanded adds the expanded class to any button regardless of a11y', () => { - const { container } = render(); + it('when expanded adds the expanded class to the button and expandIcon', () => { + const { container, getByRole } = render(); - expect(container.firstChild).to.have.class(classes.expanded); - expect(container.firstChild).to.have.attribute('aria-expanded', 'true'); - expect(container.firstChild).not.to.be.inaccessible; + const button = getByRole('button'); + expect(button).to.have.class(classes.expanded); + expect(button).to.have.attribute('aria-expanded', 'true'); expect(container.querySelector(`.${classes.expandIcon}`)).to.have.class(classes.expanded); - expect(container.querySelector(`.${classes.expandIcon}`)).to.be.inaccessible; }); - it('should render with the expand icon and have the expandIcon class', () => { + it('should render with an inaccessible expand icon and have the expandIcon class', () => { const { container } = render(Icon} />); - const expandButton = container.querySelector(`.${classes.expandIcon}`); - expect(expandButton).to.have.class(classes.expandIcon); - expect(expandButton).to.have.text('Icon'); - expect(expandButton).to.be.inaccessible; + const expandIcon = container.querySelector(`.${classes.expandIcon}`); + expect(expandIcon).to.have.text('Icon'); + expect(expandIcon).to.be.inaccessible; }); it('focusing adds the `focused` class if focused visible', () => {