-
-
Notifications
You must be signed in to change notification settings - Fork 32.3k
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] Allow expansion on icon click only #20087
[TreeView] Allow expansion on icon click only #20087
Conversation
tonyhallett
commented
Mar 12, 2020
- I have followed (at least) the PR section of the contributing guide.
Details of bundle changes.Comparing: b9c73c1...fda73b4 Details of page changes
|
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.
Would it be possible to invert the control instead? Can you prevent toggling with a by controlling expanded
and check for yourself?
You could. Seems like a fair enough feature request to separate click selection and expansion. |
Depends how hard it is to achieve. I wouldn't want to make this too easy since it's easy to create bad UX with this. The icon is really tiny and if it's the only clickable surface it doesn't meet the usual hit target. |
If the developer wants the functionality then they can restyle to make it larger. ( I personally prefer the icon larger anyway ) I don't know what you mean by
Clicking anywhere in the containing div and toggling is performed. The TreeItem internal styling could change based on the prop to make the icon larger to 24px. |
@eps1lon I am trying to open my pull request in codesandbox but cannot get it to work. Can you advise ? Do I need to have certain permissions for mui-org ? |
Should be able to open https://ci.codesandbox.io/status/mui-org/material-ui/pr/20087/builds/14476. The package urls can be used instead of version strings. The codesandbox build page also includes links to codesandboxes that use the build from this PR. |
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.
I think we should rather use the approach we use with the other components: Add labelProps
that are spread to the <Typography>{label}</Typography>
and then you can intercept clicks that happen on the label.
A single prop makes it too easy to build inaccessible treeitems. The icon does not have a sufficient hit target.
Thanks, that is exactly what I wanted. |
What are you suggesting is done in the label onClick ? Attach to the event object a boolean for use in onNodeToggle ?
As suggested, there could be a corresponding style that TreeItem would apply instead of classes.iconContainer. You seem decided on this point so I will not mention it anymore. |
replaced by #20657 |