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

[TreeView] Simplify customization #21514

Merged
merged 4 commits into from
Jun 24, 2020
Merged

[TreeView] Simplify customization #21514

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
52f526a
Add more comments
joshwooding May 6, 2020
a4647f2
Experiment with simplifying styling
joshwooding May 6, 2020
d81c2c0
docs
joshwooding Jun 20, 2020
54019eb
push further
oliviertassinari Jun 21, 2020
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
5 changes: 3 additions & 2 deletions docs/pages/api-docs/tree-item.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,11 @@ Any other props supplied will be provided to the root element (native element).
| Rule name | Global class | Description |
|:-----|:-------------|:------------|
| <span class="prop-name">root</span> | <span class="prop-name">.MuiTreeItem-root</span> | Styles applied to the root element.
| <span class="prop-name">expanded</span> | <span class="prop-name">.Mui-expanded</span> | Pseudo-class applied to the root element when expanded.
| <span class="prop-name">selected</span> | <span class="prop-name">.Mui-selected</span> | Pseudo-class applied to the root element when selected.
| <span class="prop-name">group</span> | <span class="prop-name">.MuiTreeItem-group</span> | Styles applied to the `role="group"` element.
| <span class="prop-name">content</span> | <span class="prop-name">.MuiTreeItem-content</span> | Styles applied to the tree node content.
| <span class="prop-name">expanded</span> | <span class="prop-name">.Mui-expanded</span> | Pseudo-class applied to the content element when expanded.
| <span class="prop-name">selected</span> | <span class="prop-name">.Mui-selected</span> | Pseudo-class applied to the content element when selected.
| <span class="prop-name">focused</span> | <span class="prop-name">.Mui-focused</span> | Pseudo-class applied to the content element when focused.
| <span class="prop-name">iconContainer</span> | <span class="prop-name">.MuiTreeItem-iconContainer</span> | Styles applied to the tree node icon and collapse/expand icon.
| <span class="prop-name">label</span> | <span class="prop-name">.MuiTreeItem-label</span> | Styles applied to the label element.

Expand Down
29 changes: 14 additions & 15 deletions docs/src/pages/components/tree-view/GmailTreeView.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,26 +17,23 @@ 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,
borderTopRightRadius: theme.spacing(2),
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, &$selected$focused': {
backgroundColor: `var(--tree-view-bg-color, ${theme.palette.action.selected})`,
color: 'var(--tree-view-color)',
},
},
group: {
marginLeft: 0,
Expand All @@ -46,14 +43,15 @@ const useTreeItemStyles = makeStyles((theme) => ({
},
expanded: {},
selected: {},
focused: {},
label: {
fontWeight: 'inherit',
color: 'inherit',
},
labelRoot: {
display: 'flex',
alignItems: 'center',
padding: theme.spacing(0.5, 0),
padding: theme.spacing(0.5, 0, 0.5, 0.5),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Included from the other PR.

},
labelIcon: {
marginRight: theme.spacing(1),
Expand All @@ -67,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;

Expand All @@ -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,
}}
Expand Down
29 changes: 14 additions & 15 deletions docs/src/pages/components/tree-view/GmailTreeView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,26 +33,23 @@ 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,
borderTopRightRadius: theme.spacing(2),
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, &$selected$focused': {
backgroundColor: `var(--tree-view-bg-color, ${theme.palette.action.selected})`,
color: 'var(--tree-view-color)',
},
},
group: {
marginLeft: 0,
Expand All @@ -62,14 +59,15 @@ const useTreeItemStyles = makeStyles((theme: Theme) =>
},
expanded: {},
selected: {},
focused: {},
label: {
fontWeight: 'inherit',
color: 'inherit',
},
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),
Expand All @@ -84,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;

Expand All @@ -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,
}}
Expand Down
12 changes: 9 additions & 3 deletions packages/material-ui-lab/src/PaginationItem/PaginationItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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)': {
Expand All @@ -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)': {
Expand Down
1 change: 1 addition & 0 deletions packages/material-ui-lab/src/TreeItem/TreeItem.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@ export type TreeItemClassKey =
| 'root'
| 'expanded'
| 'selected'
| 'focused'
| 'group'
| 'content'
| 'iconContainer'
Expand Down
83 changes: 49 additions & 34 deletions packages/material-ui-lab/src/TreeItem/TreeItem.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,28 +15,7 @@ export const styles = (theme) => ({
margin: 0,
padding: 0,
outline: 0,
WebkitTapHighlightColor: 'transparent',
'&:focus > $content $label': {
backgroundColor: theme.palette.action.hover,
},
'&$selected > $content $label': {
backgroundColor: fade(theme.palette.primary.main, theme.palette.action.selectedOpacity),
},
'&$selected > $content $label:hover, &$selected:focus > $content $label': {
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',
},
},
},
/* Pseudo-class applied to the root element when expanded. */
expanded: {},
/* Pseudo-class applied to the root element when selected. */
selected: {},
/* Styles applied to the `role="group"` element. */
group: {
margin: 0,
Expand All @@ -49,7 +28,43 @@ export const styles = (theme) => ({
display: 'flex',
alignItems: 'center',
cursor: 'pointer',
WebkitTapHighlightColor: 'transparent',
'&: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.focus,
},
'&$selected': {
backgroundColor: fade(theme.palette.primary.main, theme.palette.action.selectedOpacity),
'&: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,
),
},
},
},
/* Pseudo-class applied to the content element when expanded. */
expanded: {},
/* Pseudo-class applied to the content element when selected. */
selected: {},
/* Pseudo-class applied to the content element when focused. */
focused: {},
/* Styles applied to the tree node icon and collapse/expand icon. */
iconContainer: {
marginRight: 4,
Expand All @@ -66,13 +81,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',
},
},
},
});

Expand Down Expand Up @@ -189,6 +197,7 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {

const handleMouseDown = (event) => {
if (event.shiftKey || event.ctrlKey || event.metaKey) {
// Prevent text selection
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this also prevent focus in certain browsers or do those only focus if no meta keys are pressed?

Copy link
Member Author

@joshwooding joshwooding Jun 24, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right but because we don't depend on the focusing of a tree item for anything since we call focus in the click handler it shouldn't matter. But it might trip up someone depending on this behaviour. I have plans to change this part of the tree anyway.

event.preventDefault();
}

Expand Down Expand Up @@ -363,16 +372,18 @@ 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;
}

return (
<li
className={clsx(classes.root, className, {
[classes.expanded]: expanded,
[classes.selected]: selected,
})}
className={clsx(classes.root, className)}
role="treeitem"
onKeyDown={handleKeyDown}
onFocus={handleFocus}
Expand All @@ -383,7 +394,11 @@ const TreeItem = React.forwardRef(function TreeItem(props, ref) {
{...other}
>
<div
className={classes.content}
className={clsx(classes.content, {
[classes.expanded]: expanded,
[classes.selected]: selected,
[classes.focused]: focused,
})}
onClick={handleClick}
onMouseDown={handleMouseDown}
ref={contentRef}
Expand Down
1 change: 0 additions & 1 deletion packages/material-ui-lab/src/TreeItem/TreeItem.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -928,7 +928,6 @@ describe('<TreeItem />', () => {
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', () => {
Expand Down