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

[Treeview] Add node selection support #18357

Merged
merged 58 commits into from
Feb 25, 2020

Conversation

joshwooding
Copy link
Member

@joshwooding joshwooding commented Nov 13, 2019

Closes #16795

Adds node selection on mouse events and keyboard events following the wai-aria practices.

Preview: https://deploy-preview-18357--material-ui.netlify.com/components/tree-view/

@joshwooding joshwooding added new feature New feature or request component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! labels Nov 13, 2019
@mui-pr-bot
Copy link

mui-pr-bot commented Nov 13, 2019

@material-ui/lab: parsed: +1.66% , gzip: +1.41%

Details of bundle changes.

Comparing: 7139c42...25ae073

bundle Size Change Size Gzip Change Gzip
@material-ui/lab ▲ +3.21 kB (+1.66% ) 197 kB ▲ +806 B (+1.41% ) 58.1 kB
TreeView ▲ +1.97 kB (+2.93% ) 69 kB ▲ +523 B (+2.48% ) 21.6 kB
TreeItem ▲ +1.25 kB (+1.68% ) 75.6 kB ▲ +356 B (+1.51% ) 23.9 kB
SpeedDialAction -- 119 kB ▲ +5 B (+0.01% ) 37.6 kB
Slider -- 77 kB ▲ +3 B (+0.01% ) 24.2 kB
Box -- 72.2 kB ▲ +2 B (+0.01% ) 21.9 kB
Checkbox -- 83.4 kB ▲ +2 B (+0.01% ) 26.3 kB
Pagination -- 85.6 kB ▲ +2 B (+0.01% ) 26.3 kB
Radio -- 84.4 kB ▲ +2 B (+0.01% ) 26.6 kB
Select -- 117 kB ▼ -2 B (-0.01% ) 34.7 kB
TextField -- 126 kB ▼ -2 B (-0.01% ) 36.7 kB
@material-ui/core -- 362 kB ▼ -1 B (-0.00% ) 99 kB
@material-ui/system -- 16.5 kB ▼ -1 B (-0.02% ) 4.32 kB
Autocomplete -- 132 kB ▲ +1 B (0.00% ) 41.5 kB
RadioGroup -- 64.8 kB ▲ +1 B (0.00% ) 20.1 kB
Switch -- 82.6 kB ▼ -1 B (-0.00% ) 26 kB
TablePagination -- 144 kB ▼ -1 B (-0.00% ) 42 kB
useAutocomplete -- 14.8 kB ▲ +1 B (+0.02% ) 5.34 kB
@material-ui/core[umd] -- 318 kB -- 92.2 kB
@material-ui/styles -- 51.4 kB -- 15.4 kB
Alert -- 83.8 kB -- 26.3 kB
AlertTitle -- 64.5 kB -- 20.3 kB
AppBar -- 64.4 kB -- 20.2 kB
Avatar -- 65.6 kB -- 20.7 kB
AvatarGroup -- 62.7 kB -- 19.7 kB
Backdrop -- 68.2 kB -- 21.1 kB
Badge -- 65.7 kB -- 20.4 kB
BottomNavigation -- 62.7 kB -- 19.7 kB
BottomNavigationAction -- 75.9 kB -- 24 kB
Breadcrumbs -- 80.8 kB -- 25.4 kB
Button -- 80.1 kB -- 24.5 kB
ButtonBase -- 74.4 kB -- 23.3 kB
ButtonGroup -- 83.6 kB -- 25.6 kB
Card -- 63.2 kB -- 19.8 kB
CardActionArea -- 75.5 kB -- 23.8 kB
CardActions -- 62.4 kB -- 19.6 kB
CardContent -- 62.3 kB -- 19.5 kB
CardHeader -- 65.4 kB -- 20.6 kB
CardMedia -- 62.7 kB -- 19.7 kB
Chip -- 83 kB -- 25.4 kB
CircularProgress -- 64.4 kB -- 20.3 kB
ClickAwayListener -- 3.91 kB -- 1.55 kB
Collapse -- 68.4 kB -- 21.2 kB
colorManipulator -- 3.88 kB -- 1.52 kB
Container -- 63.5 kB -- 19.9 kB
CssBaseline -- 62.3 kB -- 19.6 kB
Dialog -- 83.4 kB -- 26 kB
DialogActions -- 62.4 kB -- 19.6 kB
DialogContent -- 62.6 kB -- 19.6 kB
DialogContentText -- 64.4 kB -- 20.2 kB
DialogTitle -- 64.6 kB -- 20.3 kB
Divider -- 63 kB -- 19.8 kB
docs.landing -- 56.8 kB -- 15.6 kB
docs.main -- 602 kB -- 195 kB
Drawer -- 85.2 kB -- 25.9 kB
ExpansionPanel -- 72.7 kB -- 22.7 kB
ExpansionPanelActions -- 62.4 kB -- 19.6 kB
ExpansionPanelDetails -- 62.3 kB -- 19.5 kB
ExpansionPanelSummary -- 78.5 kB -- 24.8 kB
Fab -- 77.2 kB -- 24.1 kB
Fade -- 23.6 kB -- 8.01 kB
FilledInput -- 74 kB -- 23 kB
FormControl -- 64.8 kB -- 20.2 kB
FormControlLabel -- 65.9 kB -- 20.6 kB
FormGroup -- 62.3 kB -- 19.6 kB
FormHelperText -- 63.7 kB -- 20 kB
FormLabel -- 63.8 kB -- 19.8 kB
Grid -- 65.4 kB -- 20.5 kB
GridList -- 62.8 kB -- 19.7 kB
GridListTile -- 64.1 kB -- 20.1 kB
GridListTileBar -- 63.6 kB -- 19.9 kB
Grow -- 24.2 kB -- 8.22 kB
Hidden -- 66.3 kB -- 20.8 kB
Icon -- 63.1 kB -- 19.8 kB
IconButton -- 76.6 kB -- 23.9 kB
Input -- 73 kB -- 22.8 kB
InputAdornment -- 65.4 kB -- 20.6 kB
InputBase -- 71.1 kB -- 22.3 kB
InputLabel -- 65.7 kB -- 20.5 kB
LinearProgress -- 65.7 kB -- 20.5 kB
Link -- 67 kB -- 21.1 kB
List -- 62.7 kB -- 19.6 kB
ListItem -- 77.6 kB -- 24.2 kB
ListItemAvatar -- 62.5 kB -- 19.5 kB
ListItemIcon -- 62.5 kB -- 19.6 kB
ListItemSecondaryAction -- 62.3 kB -- 19.5 kB
ListItemText -- 65.3 kB -- 20.5 kB
ListSubheader -- 63.1 kB -- 19.8 kB
Menu -- 89.1 kB -- 27.4 kB
MenuItem -- 78.6 kB -- 24.5 kB
MenuList -- 66.4 kB -- 20.8 kB
MobileStepper -- 68.2 kB -- 21.4 kB
Modal -- 14.5 kB -- 5.04 kB
NativeSelect -- 77.3 kB -- 24.4 kB
NoSsr -- 2.19 kB -- 1.04 kB
OutlinedInput -- 75 kB -- 23.4 kB
PaginationItem -- 81.2 kB -- 25 kB
Paper -- 62.7 kB -- 19.6 kB
Popover -- 83.5 kB -- 25.8 kB
Popper -- 28.8 kB -- 10.3 kB
Portal -- 2.92 kB -- 1.3 kB
Rating -- 70.8 kB -- 22.8 kB
RootRef -- 4.24 kB -- 1.64 kB
ScopedCssBaseline -- 63.2 kB -- 19.9 kB
Skeleton -- 63.3 kB -- 20 kB
Slide -- 25.7 kB -- 8.74 kB
Snackbar -- 75.7 kB -- 23.7 kB
SnackbarContent -- 63.9 kB -- 20.1 kB
SpeedDial -- 86.7 kB -- 27.3 kB
SpeedDialIcon -- 64.9 kB -- 20.3 kB
Step -- 63 kB -- 19.8 kB
StepButton -- 82.8 kB -- 26.2 kB
StepConnector -- 63.1 kB -- 19.9 kB
StepContent -- 69.5 kB -- 21.8 kB
StepIcon -- 65 kB -- 20.3 kB
StepLabel -- 69 kB -- 21.7 kB
Stepper -- 65.2 kB -- 20.6 kB
styles/createMuiTheme -- 16.6 kB -- 5.85 kB
SvgIcon -- 63.4 kB -- 19.8 kB
SwipeableDrawer -- 92.6 kB -- 28.9 kB
Tab -- 76.7 kB -- 24.3 kB
Table -- 62.9 kB -- 19.7 kB
TableBody -- 62.4 kB -- 19.5 kB
TableCell -- 64.4 kB -- 20.3 kB
TableContainer -- 62.3 kB -- 19.5 kB
TableFooter -- 62.5 kB -- 19.5 kB
TableHead -- 62.4 kB -- 19.5 kB
TableRow -- 62.8 kB -- 19.7 kB
TableSortLabel -- 77.8 kB -- 24.4 kB
Tabs -- 85.8 kB -- 27.2 kB
TextareaAutosize -- 5.24 kB -- 2.18 kB
ToggleButton -- 76.6 kB -- 24.2 kB
ToggleButtonGroup -- 63.5 kB -- 20 kB
Toolbar -- 62.7 kB -- 19.7 kB
Tooltip -- 103 kB -- 32.4 kB
Typography -- 64 kB -- 20 kB
useMediaQuery -- 2.58 kB -- 1.06 kB
Zoom -- 23.6 kB -- 8.12 kB

Generated by 🚫 dangerJS against 25ae073

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

It's pretty slick! I would propose to explore these dimensions in the future:

  • In the table of content, navigation, it's not obvious we have a first single select demo. I would propose the introduction of a h2 for it. Maybe: ## Simple tree view? I think that we can standardize it at whole documentation level. Most of the time, we need to have a first simple demo and to give it a h2. cc @mbrookes
  • We might want to work on the visual aspect of the selected and focused states
    Capture d’écran 2019-11-21 à 12 50 13
    I'm not aware of any recommendations from the material design spec, unfortunately. I would propose we handle this component like the menu/autocomplete ones [Menu] Improve hover/select/focus UI display #5186.
  • I think that we should update the default demos to include leaves and not-leaves at the same level. This is related to the comment in: [docs] Replace react-inspector with custom TreeView implementation #17662 (review). We might have an indentation issue.

@mbrookes
Copy link
Member

I would propose the introduction of a h2 for it. Maybe: ## Simple tree view?

👍 At some point though I would like to change "Simple" to "Basic" (a basic example is not always so simple!)

Also, I guess the controlled demo should be moved out of the "Customised" section.

For multi-select, is using a checkbox rather than node highlighting to follow? (For example selecting a branch node selecting all children; selecting less than all children part-selecting the parent node; behaviour for a disabled branch.)

We might want to work on the visual aspect of the selected and focused states

As well as a change of colour, it might help to include the dropdown icon (with some left padding) in the highlight:

Vuetify:

image

Or at least some left padding so that the text isn't butting up against the highlight:

Ant:

image

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 21, 2019

a basic example is not always so simple!

Fair enough.

Also, I guess the controlled demo should be moved out of the "Customised" section.

Agree, I did take care of it in #18396.

For multi-select, is using a checkbox rather than node highlighting to follow?

I think that it would be great to add checkbox support too, however, I would encourage to benchmark the list of external implementations I could find in #16795 (comment). It seems that (multi)selection and checkbox cover different use cases.

@mbrookes
Copy link
Member

mbrookes commented Nov 22, 2019

I would encourage to benchmark the list of external implementations I could find

From what I could tell, of the four that have examples of multi-select, two used node selection, two used checkboxes, none of those showed both.

Checkbox seems to be a more flexible option (select a branch, for example) for the same use case?

If we can support both versions with little overhead, then why not?

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 23, 2019

@mbrookes I could find the following demos with multi-selection:

multi-select with node selection

multi-select with checkbox selection and node selection

I would be in favor of simplifying the logic if possible. For instance, if we can isolate the checkbox and node selection behavior, I think that it would be great.

@joshwooding
Copy link
Member Author

joshwooding commented Nov 24, 2019

I think that we should update the default demos to include leaves and not-leaves at the same level. This is related to the comment in: #17662 (review). We might have an indentation issue.

@oliviertassinari What do you think of @eps1lon's concerns in #17662 (comment) ?

Although looking at Webstorm, the file tree does this anyway.

@joshwooding
Copy link
Member Author

joshwooding commented Nov 24, 2019

Experimented with the indentation. This is the outcome:

Before
image
After
image

@oliviertassinari
Copy link
Member

oliviertassinari commented Nov 25, 2019

I'm not adding this as the default since this would add a margin to leaf-only groups.

@joshwooding I think that it's fine, it would lead to more consistency. However, I think that we can explore the following:

  1. We reduce the size of the icon. Benchmarking, the icons seem a bit too large. From what I understand, you took inspiration from Windows file folder manager, I believe the icon is smaller.
  2. I think that we should exclude the icon from the selection state or include the whole list (Gmail, macOS, Google Drive)

Regarding the color issue, I think that a rebase on master so we can see the tree view on a white background would help to evaluate. Looking at the other implementation, a colorful selected state seems frequent, e.g. Storybook:

Capture d’écran 2019-11-25 à 12 13 46

Or even in Google Drive. But in the material design spec, it's not that obvious. Your approach sounds good to me, I think that we can move forward with it.

@mbrookes
Copy link
Member

I did take care of it in #18396.

You did, but now it's a sub-section of Multi-select 😄

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Love it. The selector isn't ideal yet but we can probably improve that later.

@oliviertassinari oliviertassinari merged commit 92c66df into mui:master Feb 25, 2020
@oliviertassinari
Copy link
Member

A truly great piece of work!

@joshwooding
Copy link
Member Author

It's finally done! Until someone finds a bug... but still done 😁

@joshwooding joshwooding deleted the tree-view-selection branch February 25, 2020 20:37
@Devil005
Copy link

Thanks for releasing this feature. Could you let me know in which version I can get these new actions on TreeView? I am currently using

"@material-ui/core": "^4.9.4", "@material-ui/lab": "^4.0.0-alpha.44"

@joshwooding
Copy link
Member Author

@Devil005 The new actions will be available in 4.0.0-alpha.45 - We haven’t released it yet but I believe it will be released soon.

@bshin19
Copy link

bshin19 commented Apr 1, 2020

this change made in gmail-treeview doesn't appear to do what it's supposed to do, causing the pattern to no longer match the gmail style (ugly box around content)

'&:focus > $content $label, &:hover > $content $label, &$selected > $content $label': { backgroundColor: 'transparent', },

it should be
'&$selected > $content > $label:hover, &$selected:focus > $content > $label': { backgroundColor: 'transparent' }

based on the styles it's trying to override.

@oliviertassinari
Copy link
Member

@bshin19 Thanks for mentioning it, we have an effort related to this problem in #20159.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Treeview] Node selection support
8 participants