-
-
Notifications
You must be signed in to change notification settings - Fork 1.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] Allow to customize the indentation of nested items #13225
[TreeView] Allow to customize the indentation of nested items #13225
Conversation
pathname: '/x/react-tree-view/common-features', | ||
subheader: 'Common features', | ||
children: [ | ||
{ pathname: '/x/react-tree-view/tree-item-customization', title: 'Item customization' }, |
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 took the opportunity to create this page we were talking about
It will also host all the doc about useTreeItem2
usage and TreeItem2
slot usage.
Should we also move Accessibility here?
Would you put it below or above the components?
On the charts and pickers it's below so I did the same here.
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 the Accessibility page should definitely be in common features 🤔
And having this section above somehow makes more sense to me - but I'm flexible on this one
|
||
{{"demo": "ItemChildrenIndentationPxProp.js"}} | ||
|
||
:::success |
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 was super explicit with those :::success
This one will be simplified in v8 once we drop the experimental feature and the other one will just disappear with all the doc around the experimental feature.
Deploy preview: https://deploy-preview-13225--material-ui-x.netlify.app/ Updated pages: |
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.
Nice addition 🎉 Leaving a few comments 👇
pathname: '/x/react-tree-view/common-features', | ||
subheader: 'Common features', | ||
children: [ | ||
{ pathname: '/x/react-tree-view/tree-item-customization', title: 'Item customization' }, |
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 the Accessibility page should definitely be in common features 🤔
And having this section above somehow makes more sense to me - but I'm flexible on this one
* Indentation in pixels between two an item and its children. | ||
* @default 12 | ||
*/ | ||
itemChildrenIndentationPx: PropTypes.number, |
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 am not sure this should always be in px 🤔
How would we support other units? Would we have a prop for each? I think having this prop called itemChildrenIndentation
and allowing for different units as well (with a fallback to px) is a far better dx - but maybe I'm missing something in terms of feasibility
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.
There is one place in the drag & drop where we need to use the value in pixels:
if (validActions['move-to-parent'] && cursorX < params.itemChildrenIndentationPx) {
action = 'move-to-parent';
}
If we can convert the value it's fine, but it would be hard to support all units I guess.
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.
Not sure how we could handle this, because I do agree with you that forcing the usage of pixels it not optimal
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.
If we can convert the value it's fine
Oh, I see. 🤔 There is a way using a temporary element and getComputedStyle
but it's kind of hacky IMO 🙈
We can go with the px value for now I think
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 kind of hacky
And for stuff like %
or em
it would still be problematic 😬
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.
Yes, true 🙈
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.
Nice addition 🎉
* Indentation in pixels between two an item and its children. | ||
* @default 12 | ||
*/ | ||
itemChildrenIndentationPx: PropTypes.number, |
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.
If we can convert the value it's fine
Oh, I see. 🤔 There is a way using a temporary element and getComputedStyle
but it's kind of hacky IMO 🙈
We can go with the px value for now I think
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.
Looking good 👌 🎉 Small comment about the description of the prop
Extracted from #12213
Follow up on #13126