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] Node selection support #16795

Closed
2 tasks done
zjaml opened this issue Jul 29, 2019 · 32 comments · Fixed by #18357
Closed
2 tasks done

[Treeview] Node selection support #16795

zjaml opened this issue Jul 29, 2019 · 32 comments · Fixed by #18357
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

Comments

@zjaml
Copy link

zjaml commented Jul 29, 2019

Thanks very much for the release of the brand new TreeView component! ❤️
I'd like to request a basic feature for the Treeview component to emit an event when any node is selected.

You may have already planned this, but I couldn't find any information for it, hence the request.

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

A node selected event fired when a node is selected or focused.

Current Behavior 😯

No event is fired at node selection, onNodeToggle event doesn't fire for leaf nodes, onClick event can be attached to each TreeItem as a workaround, however it doesn't support keyboard action.

Examples 🌈

Context 🔦

I'm implementing something where the rest of the UI needs to be updated according to the selection of the node. This is a much needed feature.

@oliviertassinari oliviertassinari added the component: tree view TreeView, TreeItem. This is the name of the generic UI component, not the React module! label Jul 29, 2019
@oliviertassinari

This comment has been minimized.

@zjaml
Copy link
Author

zjaml commented Jul 29, 2019

@oliviertassinari onFocus does the trick for me, thanks so much : )
Your proposal looks awesome!
BTW, I became your patreon to buy you a beer.

@zjaml
Copy link
Author

zjaml commented Jul 30, 2019

Some further feedback, I found that using onFocus as a way to manage selected state less than ideal. Some downsides are:

  1. onFocus event handler need to be attached to every TreeItem, causing many duplication in code for more sophisticated rendering.
  2. event.stopPropagation() needs to be called on each onFocus handler

Overall, the user would have to think unnecessarily on the implementation of TreeView to use it.
The support for component prop have similar downsides, that's why I prefer to have a nodeSelected event on the TreeView itself.

@zjaml
Copy link
Author

zjaml commented Jul 30, 2019

There's some other bugs I encountered for TreeView, maybe I should start a new issue, but I decided to post here.

  1. Render error occurs if a null value appears in children of TreeItem.
  2. label prop of TreeItem is Node type, but render error occurs if an React element is passed.

@KaRkY
Copy link
Contributor

KaRkY commented Jul 31, 2019

@oliviertassinari How could the onFocus work if you want to select parent without closing the node?

@joshwooding
Copy link
Member

Would a checkbox TreeView solve this?

@KaRkY
Copy link
Contributor

KaRkY commented Jul 31, 2019

@joshwooding Not always sometimes a radio would be better if there is select only 1. Think of files and folders with details panel.

@zjaml
Copy link
Author

zjaml commented Jul 31, 2019

The TreeView needs to have a state to manage selected nodes ideally controllable from a prop.
Notice selected state is different from the focus state, as focus can easily get lost when the user clicks somewhere outside of the TreeView.

For example, in VSCode, if you select a file to open the editor and go edit the file, the file item in the treeview lost focus, however it's still selected.

Sometimes the selection in the treeview needs to sync to external events, for example, VSCode's treeview sync to current active file.

Ideally when receiving the selected node prop, the treeview needs to perform a minimum change to reflect the new selected state, leaving previous expansion state by the user untouched.

To sum it up, I think a prop to control selected nodes and an event to notice nodes being selected are what's missing.

@oliviertassinari
Copy link
Member

onFocus event handler need to be attached to every TreeItem, causing many duplication in code for more sophisticated rendering.

I would only listen to the focus event at the root of the tree view. The focus event bubbles.

To sum it up, I think a prop to control selected nodes and an event to notice nodes being selected are what's missing.

The use case seems valid to me 👍.

@joshwooding joshwooding added the new feature New feature or request label Aug 6, 2019
@baig
Copy link
Contributor

baig commented Aug 6, 2019

Is somebody already working on this? I can work on this issue.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 7, 2019

@baig I'm not aware of anyone working on the issue. I think that it's clear to go 💃. What solution do you envision? Here is a proposal API:

selected?: string[]; // an array, e.g. in the file system, we can select multiple items.
onSelect?: (event, nodeId: string)

cc @joshwooding

But I have a couple of questions:

  • What would trigger the select event? Only the click event or the keyboard event too? I'm tempted to say both.
  • How is the select style different from the focus one? Maybe it should be identical by default?
  • Should we only have a controlled mode? Or should we also a prop to enable an uncontrolled mode, defaultSelected?: string[]? For benchmark: https://vuetifyjs.com/en/components/treeview#activatable? I would say yes.

@baig
Copy link
Contributor

baig commented Aug 8, 2019

@oliviertassinari The proposed API makes sense to me.

My answer to your questions:

What would trigger the select event? Only the click event or the keyboard event too? I'm tempted to say both.

Ideally both the click and the keyboard event should trigger the select event. We can also complete it in steps and add the support for the keyboard event later.

How is the select style different from the focus one? Maybe it should be identical by default?

I think it should be identical by default. If valid use cases pop up, then we can add support for customizing focus/select styles.

Should we only have a controlled mode? Or should we also a prop to enable an uncontrolled mode, defaultSelected?: string[]? For benchmark: https://vuetifyjs.com/en/components/treeview#activatable? I would say yes.

I'll second you by saying that we should support both modes. Again, we can add support for the uncontrolled mode later.

@joshwooding
Copy link
Member

joshwooding commented Aug 8, 2019

There is a section in the aria practices that discusses selection keyboard interaction on a multi select tree but nothing mentioning click specifically or even single select

@oliviertassinari
Copy link
Member

@joshwooding Thanks for linking the wai-aria 📚! It's a gold mine ⛏. They seem to encourage the "selection follows focus" approach, unless it slows down the UX. When the approach is not used, the selection and focus state should have different styles. (also related to the tabs cc @mbrookes)

@baig I think that this new information confirms that we are on the right direction.

@baig
Copy link
Contributor

baig commented Aug 8, 2019

@oliviertassinari I'll start working on the implementation. You can assign this task to me.

@baig
Copy link
Contributor

baig commented Aug 8, 2019

While working on the implementation, few questions have popped up:

  1. Can nodes and leaves both be selected? If nodes can be selected, should it select all the children as well?
  2. Should there be a prop to enable multi-selection? I think the tree should be a single select tree by default.

@oliviertassinari
Copy link
Member

  1. It the case is a typical file system manager
  2. Should we leave multi-selection for another pull request? Start simple?

@baig
Copy link
Contributor

baig commented Aug 8, 2019

If the use case is a typical file manager...

  1. ...then we allow selecting both the nodes (without selecting its children) and the leaves.
  2. ...multi-selection will be a fairly common use-case.

@oliviertassinari
Copy link
Member

oliviertassinari commented Aug 8, 2019

  1. Oh sorry, you said both. I think that it should only select the item, not the children. One at the time.
  2. The multi-selection will probably require the handling of the command, ctrl and shift keys (I have already implemented it in the past, I can share the logic). It could work without a custom prop. I think that it depends on how we want to structure the API. The only way I see it work without a prop is to have the onSelect event report the whole selection state of the tree, not just the new item. In this case, somebody controlling the tree can pick the last item of the array to disable the multi-selection behavior. But what's most common? I would imagine more people using a single selection behavior. I think that a prop, in this case, would be better.

So what about?:

selected?: string | string[]; // an array, when multiselect is true
defaultSelected?: string | string[];
onSelect?: (event, nodeId: string | string[]) => void;
multiselect?: boolean; // false by default

@mbrookes
Copy link
Member

mbrookes commented Aug 8, 2019

also related to the tabs

Yes - as you say, in the case of tabs, it is recommended in the aria practices that the active tab follows focus, unless this would introduce latency when switching tabs (such as downloading the tab content), in which case changing the active tab should be in response to a key-press.

Where a tree view is controlling the displayed content (most cases?), it is essentially equivalent to nested vertical tabs, so the same approach would apply.

@switz
Copy link

switz commented Sep 3, 2019

Checking in on this to see if any progress has made, if so, maybe I'll run with it, feel free to share a branch.

@baig
Copy link
Contributor

baig commented Sep 6, 2019

I'll share my work over the weekend. Ideally I'd like to wrap it up myself but I've been busy lately and couldn't work on it.

@baig
Copy link
Contributor

baig commented Sep 13, 2019

@switz I'm resuming work on this issue this weekend. I'll share the branch too. Sorry, couldn't do it last week.

@baig
Copy link
Contributor

baig commented Sep 16, 2019

@oliviertassinari

  1. My question was probably phrased poorly but I understood your answer. I wanted to clarify if we allow selection of nodes or not.
  2. I think adding a prop to enable/disable multi-selection is better irrespective of the most common use case. The suggested props look good to me.

@oisinlavery
Copy link

I opened a related feature request here: #17702

After playing around with the problem, I managed to control selection state without focus by overriding the label component's onClick and TreeItem's onKeyDown:
https://codesandbox.io/s/material-ui-treeview-bespoke-selection-behaviour-tful3

You could achieve multi-select in a similar way. I still think a more powerful API would be great though.

@abedlubis
Copy link

is there any way to keep the tree-view always expanded even the node is clicked?

@joshwooding
Copy link
Member

Sneak peak:

image

@oliviertassinari oliviertassinari changed the title Node selection event support for Treeview component [Treeview] Node selection support Nov 13, 2019
@soundyogi
Copy link

Hi!

I need to programmatically update selected leaf nodes when scrolling through a document.
WIll this be possible with this MR?

Thanks :)

@marcusjwhelan
Copy link

@oliviertassinari @joshwooding

I have a working version of the TreeView with a div in the TreeItem label.

This div has a Radio button that needs to be selected. I can tell what is selected by a using react's useState with an object where the filename/path is the key.

My issue is that the radio button when pressed also fires the directories to expand/close. When all I want is the radio button's event to be handles and not the whole TreeItem to execute the Collapse. I tried to change the value of the "in" parameter on the "TransitionProps" but there doesn't seem to be a way from the "TreeItem" TransitionComponent. I am used the customized tree view as an example. If need be I can create a new issue with this. However I cannot post any code examples because of the privacy of the companies code.

TLDR -> radio button needs to be pressed and not also fired off the events of the TreeItem for expansion.

@darewreck54
Copy link

Hi,

I was wondering what the status of this was? Right now when a user clicks, the color changes for the background of the selected. However, when you click anywhere outside the previous selected item gets deselected. Is there a way to prevent the deselect unless it another tree item gets selected.

When element is selected:
image

When you select anywhere outside the tree, the white selection gets deselected.

Right now i'm leveraging off the

 style={{
        '--tree-view-color': color,
        '--tree-view-bg-color': bgColor,
      }}

As a workaround, I could probably create an internal state to manage what is selected, and then apply my own style. Wonder if there is any easier way to do. this thou?

Thanks,
Derek

@NawarA
Copy link

NawarA commented Jan 20, 2020

@darewreck54 I'm trying to figure out the same thing.

As a developer, I'd like to be able to change my icon from Folder to OpenFolder when selected, and also be able to apply a nice blue background (like the one above) to all expanded nodes.

Anyone know the simplest way to do this?

lutzhelm added a commit to lutzhelm/mirador that referenced this issue Feb 13, 2020
onClick and onKeyDown should be replaced once material-ui TreeItem has
node selection support:
mui/material-ui#16795
mejackreed pushed a commit to ProjectMirador/mirador that referenced this issue Mar 5, 2020
onClick and onKeyDown should be replaced once material-ui TreeItem has
node selection support:
mui/material-ui#16795
mejackreed pushed a commit to ProjectMirador/mirador that referenced this issue Apr 17, 2020
onClick and onKeyDown should be replaced once material-ui TreeItem has
node selection support:
mui/material-ui#16795
cbeer pushed a commit to ProjectMirador/mirador that referenced this issue Apr 17, 2020
onClick and onKeyDown should be replaced once material-ui TreeItem has
node selection support:
mui/material-ui#16795
@dimoff66

This comment has been minimized.

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 a pull request may close this issue.