From 52f526a76e21ad02f8569529149806d42f52b893 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Thu, 7 May 2020 00:20:56 +0100 Subject: [PATCH 1/4] Add more comments --- packages/material-ui-lab/src/TreeItem/TreeItem.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.js b/packages/material-ui-lab/src/TreeItem/TreeItem.js index 149db4c81ede70..a9830857cac4ae 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.js @@ -189,6 +189,7 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { const handleMouseDown = (event) => { if (event.shiftKey || event.ctrlKey || event.metaKey) { + // Prevent text selection event.preventDefault(); } @@ -363,7 +364,12 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { if (multiSelect) { ariaSelected = selected; } else if (selected) { - // single-selection trees unset aria-selected + /* single-selection trees unset aria-selected on un-selected items. + * + * If the tree does not support multiple selection, aria-selected + * is set to true for the selected node and it is not present on any other node in the tree. + * Source: https://www.w3.org/TR/wai-aria-practices/#TreeView + */ ariaSelected = true; } From a4647f29c30e3f4384f96dd1a6fb822d1743b632 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Thu, 7 May 2020 00:32:38 +0100 Subject: [PATCH 2/4] Experiment with simplifying styling --- .../components/tree-view/GmailTreeView.js | 23 ++++--- .../components/tree-view/GmailTreeView.tsx | 23 ++++--- .../src/TreeItem/TreeItem.d.ts | 1 + .../material-ui-lab/src/TreeItem/TreeItem.js | 63 ++++++++++--------- .../src/TreeItem/TreeItem.test.js | 1 - 5 files changed, 56 insertions(+), 55 deletions(-) diff --git a/docs/src/pages/components/tree-view/GmailTreeView.js b/docs/src/pages/components/tree-view/GmailTreeView.js index c789a46d516762..1d7a0d29d0582d 100644 --- a/docs/src/pages/components/tree-view/GmailTreeView.js +++ b/docs/src/pages/components/tree-view/GmailTreeView.js @@ -17,16 +17,6 @@ import ArrowRightIcon from '@material-ui/icons/ArrowRight'; const useTreeItemStyles = makeStyles((theme) => ({ root: { color: theme.palette.text.secondary, - '&:hover > $content': { - backgroundColor: theme.palette.action.hover, - }, - '&:focus > $content, &$selected > $content': { - backgroundColor: `var(--tree-view-bg-color, ${theme.palette.grey[400]})`, - color: 'var(--tree-view-color)', - }, - '&:focus > $content $label, &:hover > $content $label, &$selected > $content $label': { - backgroundColor: 'transparent', - }, }, content: { color: theme.palette.text.secondary, @@ -34,9 +24,16 @@ const useTreeItemStyles = makeStyles((theme) => ({ borderBottomRightRadius: theme.spacing(2), paddingRight: theme.spacing(1), fontWeight: theme.typography.fontWeightMedium, - '$expanded > &': { + '&$expanded': { fontWeight: theme.typography.fontWeightRegular, }, + '&:hover': { + backgroundColor: theme.palette.action.hover, + }, + '&$focused, &$selected': { + backgroundColor: `var(--tree-view-bg-color, ${theme.palette.grey[400]})`, + color: 'var(--tree-view-color)', + }, }, group: { marginLeft: 0, @@ -46,6 +43,7 @@ const useTreeItemStyles = makeStyles((theme) => ({ }, expanded: {}, selected: {}, + focused: {}, label: { fontWeight: 'inherit', color: 'inherit', @@ -53,7 +51,7 @@ const useTreeItemStyles = makeStyles((theme) => ({ labelRoot: { display: 'flex', alignItems: 'center', - padding: theme.spacing(0.5, 0), + padding: theme.spacing(0.5, 0, 0.5, 0.5), }, labelIcon: { marginRight: theme.spacing(1), @@ -97,6 +95,7 @@ function StyledTreeItem(props) { content: classes.content, expanded: classes.expanded, selected: classes.selected, + focused: classes.focused, group: classes.group, label: classes.label, }} diff --git a/docs/src/pages/components/tree-view/GmailTreeView.tsx b/docs/src/pages/components/tree-view/GmailTreeView.tsx index 626e386080b432..cf690ce2380ef5 100644 --- a/docs/src/pages/components/tree-view/GmailTreeView.tsx +++ b/docs/src/pages/components/tree-view/GmailTreeView.tsx @@ -33,16 +33,6 @@ const useTreeItemStyles = makeStyles((theme: Theme) => createStyles({ root: { color: theme.palette.text.secondary, - '&:hover > $content': { - backgroundColor: theme.palette.action.hover, - }, - '&:focus > $content, &$selected > $content': { - backgroundColor: `var(--tree-view-bg-color, ${theme.palette.grey[400]})`, - color: 'var(--tree-view-color)', - }, - '&:focus > $content $label, &:hover > $content $label, &$selected > $content $label': { - backgroundColor: 'transparent', - }, }, content: { color: theme.palette.text.secondary, @@ -50,9 +40,16 @@ const useTreeItemStyles = makeStyles((theme: Theme) => borderBottomRightRadius: theme.spacing(2), paddingRight: theme.spacing(1), fontWeight: theme.typography.fontWeightMedium, - '$expanded > &': { + '&$expanded': { fontWeight: theme.typography.fontWeightRegular, }, + '&:hover': { + backgroundColor: theme.palette.action.hover, + }, + '&$focused, &$selected': { + backgroundColor: `var(--tree-view-bg-color, ${theme.palette.grey[400]})`, + color: 'var(--tree-view-color)', + }, }, group: { marginLeft: 0, @@ -62,6 +59,7 @@ const useTreeItemStyles = makeStyles((theme: Theme) => }, expanded: {}, selected: {}, + focused: {}, label: { fontWeight: 'inherit', color: 'inherit', @@ -69,7 +67,7 @@ const useTreeItemStyles = makeStyles((theme: Theme) => labelRoot: { display: 'flex', alignItems: 'center', - padding: theme.spacing(0.5, 0), + padding: theme.spacing(0.5, 0, 0.5, 0.5), }, labelIcon: { marginRight: theme.spacing(1), @@ -114,6 +112,7 @@ function StyledTreeItem(props: StyledTreeItemProps) { content: classes.content, expanded: classes.expanded, selected: classes.selected, + focused: classes.focused, group: classes.group, label: classes.label, }} diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.d.ts b/packages/material-ui-lab/src/TreeItem/TreeItem.d.ts index aff614ab7ddbbb..5d57784f47d147 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.d.ts +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.d.ts @@ -55,6 +55,7 @@ export type TreeItemClassKey = | 'root' | 'expanded' | 'selected' + | 'focused' | 'group' | 'content' | 'iconContainer' diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.js b/packages/material-ui-lab/src/TreeItem/TreeItem.js index a9830857cac4ae..f166a942bbd4a1 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.js @@ -15,14 +15,34 @@ export const styles = (theme) => ({ margin: 0, padding: 0, outline: 0, + }, + /* Styles applied to the `role="group"` element. */ + group: { + margin: 0, + padding: 0, + marginLeft: 17, + }, + /* Styles applied to the tree node content. */ + content: { + width: '100%', + display: 'flex', + alignItems: 'center', + cursor: 'pointer', WebkitTapHighlightColor: 'transparent', - '&:focus > $content $label': { + '&:hover': { + backgroundColor: theme.palette.action.hover, + // Reset on touch devices, it doesn't add specificity + '@media (hover: none)': { + backgroundColor: 'transparent', + }, + }, + '&$focused': { backgroundColor: theme.palette.action.hover, }, - '&$selected > $content $label': { + '&$selected': { backgroundColor: fade(theme.palette.primary.main, theme.palette.action.selectedOpacity), }, - '&$selected > $content $label:hover, &$selected:focus > $content $label': { + '&$selected:hover, &$selected$focused': { backgroundColor: fade( theme.palette.primary.main, theme.palette.action.selectedOpacity + theme.palette.action.hoverOpacity, @@ -33,23 +53,12 @@ export const styles = (theme) => ({ }, }, }, - /* Pseudo-class applied to the root element when expanded. */ + /* Pseudo-class applied to the content element when expanded. */ expanded: {}, - /* Pseudo-class applied to the root element when selected. */ + /* Pseudo-class applied to the content element when selected. */ selected: {}, - /* Styles applied to the `role="group"` element. */ - group: { - margin: 0, - padding: 0, - marginLeft: 17, - }, - /* Styles applied to the tree node content. */ - content: { - width: '100%', - display: 'flex', - alignItems: 'center', - cursor: 'pointer', - }, + /* Pseudo-class applied to the content element when focused. */ + focused: {}, /* Styles applied to the tree node icon and collapse/expand icon. */ iconContainer: { marginRight: 4, @@ -66,13 +75,6 @@ export const styles = (theme) => ({ width: '100%', paddingLeft: 4, position: 'relative', - '&:hover': { - backgroundColor: theme.palette.action.hover, - // Reset on touch devices, it doesn't add specificity - '@media (hover: none)': { - backgroundColor: 'transparent', - }, - }, }, }); @@ -375,10 +377,7 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) { return (
  • ', () => { fireEvent.keyDown(getByTestId('one'), { key: ' ' }); expect(getByTestId('one')).to.have.attribute('aria-selected', 'true'); - expect(getByTestId('one')).to.have.class('Mui-selected'); }); it('should not select a node when space is pressed and disableSelection', () => { From d81c2c0ec5eba72857c354736447b2a13db96a68 Mon Sep 17 00:00:00 2001 From: joshwooding <12938082+joshwooding@users.noreply.github.com> Date: Sat, 20 Jun 2020 16:15:09 +0100 Subject: [PATCH 3/4] docs --- docs/pages/api-docs/tree-item.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/pages/api-docs/tree-item.md b/docs/pages/api-docs/tree-item.md index 69ac352d35e1d7..07764b0d7157a1 100644 --- a/docs/pages/api-docs/tree-item.md +++ b/docs/pages/api-docs/tree-item.md @@ -50,10 +50,11 @@ Any other props supplied will be provided to the root element (native element). | Rule name | Global class | Description | |:-----|:-------------|:------------| | root | .MuiTreeItem-root | Styles applied to the root element. -| expanded | .Mui-expanded | Pseudo-class applied to the root element when expanded. -| selected | .Mui-selected | Pseudo-class applied to the root element when selected. | group | .MuiTreeItem-group | Styles applied to the `role="group"` element. | content | .MuiTreeItem-content | Styles applied to the tree node content. +| expanded | .Mui-expanded | Pseudo-class applied to the content element when expanded. +| selected | .Mui-selected | Pseudo-class applied to the content element when selected. +| focused | .Mui-focused | Pseudo-class applied to the content element when focused. | iconContainer | .MuiTreeItem-iconContainer | Styles applied to the tree node icon and collapse/expand icon. | label | .MuiTreeItem-label | Styles applied to the label element. From 54019eb7990bb22bc05eaa81d70bd639aef9ff43 Mon Sep 17 00:00:00 2001 From: Olivier Tassinari Date: Sun, 21 Jun 2020 12:17:44 +0200 Subject: [PATCH 4/4] push further --- .../components/tree-view/GmailTreeView.js | 10 +++---- .../components/tree-view/GmailTreeView.tsx | 10 +++---- .../src/PaginationItem/PaginationItem.js | 12 ++++++--- .../material-ui-lab/src/TreeItem/TreeItem.js | 26 ++++++++++++------- 4 files changed, 35 insertions(+), 23 deletions(-) diff --git a/docs/src/pages/components/tree-view/GmailTreeView.js b/docs/src/pages/components/tree-view/GmailTreeView.js index 1d7a0d29d0582d..bc5ec2774e0491 100644 --- a/docs/src/pages/components/tree-view/GmailTreeView.js +++ b/docs/src/pages/components/tree-view/GmailTreeView.js @@ -30,8 +30,8 @@ const useTreeItemStyles = makeStyles((theme) => ({ '&:hover': { backgroundColor: theme.palette.action.hover, }, - '&$focused, &$selected': { - backgroundColor: `var(--tree-view-bg-color, ${theme.palette.grey[400]})`, + '&$focused, &$selected, &$selected$focused': { + backgroundColor: `var(--tree-view-bg-color, ${theme.palette.action.selected})`, color: 'var(--tree-view-color)', }, }, @@ -65,11 +65,11 @@ const useTreeItemStyles = makeStyles((theme) => ({ function StyledTreeItem(props) { const classes = useTreeItemStyles(); const { - labelText, + bgColor, + color, labelIcon: LabelIcon, labelInfo, - color, - bgColor, + labelText, ...other } = props; diff --git a/docs/src/pages/components/tree-view/GmailTreeView.tsx b/docs/src/pages/components/tree-view/GmailTreeView.tsx index cf690ce2380ef5..0dba1910ecbfca 100644 --- a/docs/src/pages/components/tree-view/GmailTreeView.tsx +++ b/docs/src/pages/components/tree-view/GmailTreeView.tsx @@ -46,8 +46,8 @@ const useTreeItemStyles = makeStyles((theme: Theme) => '&:hover': { backgroundColor: theme.palette.action.hover, }, - '&$focused, &$selected': { - backgroundColor: `var(--tree-view-bg-color, ${theme.palette.grey[400]})`, + '&$focused, &$selected, &$selected$focused': { + backgroundColor: `var(--tree-view-bg-color, ${theme.palette.action.selected})`, color: 'var(--tree-view-color)', }, }, @@ -82,11 +82,11 @@ const useTreeItemStyles = makeStyles((theme: Theme) => function StyledTreeItem(props: StyledTreeItemProps) { const classes = useTreeItemStyles(); const { - labelText, + bgColor, + color, labelIcon: LabelIcon, labelInfo, - color, - bgColor, + labelText, ...other } = props; diff --git a/packages/material-ui-lab/src/PaginationItem/PaginationItem.js b/packages/material-ui-lab/src/PaginationItem/PaginationItem.js index 7133b5362392c2..bf91b0720d3aaa 100644 --- a/packages/material-ui-lab/src/PaginationItem/PaginationItem.js +++ b/packages/material-ui-lab/src/PaginationItem/PaginationItem.js @@ -39,7 +39,7 @@ export const styles = (theme) => ({ }, '&$selected': { backgroundColor: theme.palette.action.selected, - '&:hover, &$focusVisible': { + '&:hover': { backgroundColor: fade( theme.palette.action.selected, theme.palette.action.selectedOpacity + theme.palette.action.hoverOpacity, @@ -49,6 +49,12 @@ export const styles = (theme) => ({ backgroundColor: theme.palette.action.selected, }, }, + '&$focusVisible': { + backgroundColor: fade( + theme.palette.action.selected, + theme.palette.action.selectedOpacity + theme.palette.action.focusOpacity, + ), + }, '&$disabled': { opacity: 1, color: theme.palette.action.disabled, @@ -135,7 +141,7 @@ export const styles = (theme) => ({ '&:hover, &$focusVisible': { backgroundColor: fade( theme.palette.primary.main, - theme.palette.action.activatedOpacity + theme.palette.action.hoverOpacity, + theme.palette.action.activatedOpacity + theme.palette.action.focusOpacity, ), // Reset on touch devices, it doesn't add specificity '@media (hover: none)': { @@ -156,7 +162,7 @@ export const styles = (theme) => ({ '&:hover, &$focusVisible': { backgroundColor: fade( theme.palette.secondary.main, - theme.palette.action.activatedOpacity + theme.palette.action.hoverOpacity, + theme.palette.action.activatedOpacity + theme.palette.action.focusOpacity, ), // Reset on touch devices, it doesn't add specificity '@media (hover: none)': { diff --git a/packages/material-ui-lab/src/TreeItem/TreeItem.js b/packages/material-ui-lab/src/TreeItem/TreeItem.js index f166a942bbd4a1..8b8fa806a2b9da 100644 --- a/packages/material-ui-lab/src/TreeItem/TreeItem.js +++ b/packages/material-ui-lab/src/TreeItem/TreeItem.js @@ -37,19 +37,25 @@ export const styles = (theme) => ({ }, }, '&$focused': { - backgroundColor: theme.palette.action.hover, + backgroundColor: theme.palette.action.focus, }, '&$selected': { backgroundColor: fade(theme.palette.primary.main, theme.palette.action.selectedOpacity), - }, - '&$selected:hover, &$selected$focused': { - backgroundColor: fade( - theme.palette.primary.main, - theme.palette.action.selectedOpacity + theme.palette.action.hoverOpacity, - ), - // Reset on touch devices, it doesn't add specificity - '@media (hover: none)': { - backgroundColor: 'transparent', + '&:hover': { + backgroundColor: fade( + theme.palette.primary.main, + theme.palette.action.selectedOpacity + theme.palette.action.hoverOpacity, + ), + // Reset on touch devices, it doesn't add specificity + '@media (hover: none)': { + backgroundColor: fade(theme.palette.primary.main, theme.palette.action.selectedOpacity), + }, + }, + '&$focused': { + backgroundColor: fade( + theme.palette.primary.main, + theme.palette.action.selectedOpacity + theme.palette.action.focusOpacity, + ), }, }, },