-
-
Notifications
You must be signed in to change notification settings - Fork 32.4k
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
[Treeview] Add node selection support #18357
Conversation
@material-ui/lab: parsed: +1.66% , gzip: +1.41% Details of bundle changes.Comparing: 7139c42...25ae073
|
There was a problem hiding this 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
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.
👍 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.)
As well as a change of colour, it might help to include the dropdown icon (with some left padding) in the highlight: Vuetify: Or at least some left padding so that the text isn't butting up against the highlight: Ant: |
Fair enough.
Agree, I did take care of it in #18396.
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. |
Co-Authored-By: Olivier Tassinari <olivier.tassinari@gmail.com>
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? |
@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. |
@oliviertassinari What do you think of @eps1lon's concerns in #17662 (comment) ? Although looking at Webstorm, the file tree does this anyway. |
@joshwooding I think that it's fine, it would lead to more consistency. However, I think that we can explore the following:
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: 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. |
You did, but now it's a sub-section of Multi-select 😄 |
f955113
to
aabf1d6
Compare
There was a problem hiding this 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.
A truly great piece of work! |
It's finally done! Until someone finds a bug... but still done 😁 |
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
|
@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. |
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)
it should be based on the styles it's trying to override. |
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/