-
-
Notifications
You must be signed in to change notification settings - Fork 64
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
Add tree component #460
Add tree component #460
Conversation
Test Environment for snehilvj/dash-mantine-components-460 |
Hi @Godisemo Thanks so much for this PR! 🚀 Nice example app too. Here it is hosted on PyCafe I'll take a closer look in the next couple days and get back to you with more comments and feedback. In the meantime, since you obviously know a lot about both Mantine and Dash, I hope you don't mind if I ask you a question about an open item in an unrelated PR. (#458) that I'm working on right now:
|
Thank you @AnnMarieW , unfortunately I don't have experience in the part of dash related to your other PR. I updated this PR with the rest of the upstream mantine component properties as well as updated the example app in the first comment. Let me know if there is anything else that is needed when you have had time to look at it. |
A Tree component like this has been a popular request and will make a great addition to Dash. 🎉 When adding components to DMC, our goal is to be aligned as closely as possible with the features and prop names of the upstream Mantine component. It can be a little tricky to get this right. Let's take for example, the expanded feature: Here, the
However the Mantine Tree component. has the following:
And the following functions:
In the example app, if you click on a node with no children it toggles on and off the There are also similar features for the It would be great to get some feedback on the API, prop names etc from @alexcjohnson I think it's best to take a little time with the design now so we don't have breaking changes later. Update: @callback(
Output("tree", "expanded"),
Input("expand-all", "n_clicks"),
Input("collapse-all", "n_clicks")
)
def update(e,c):
if ctx.triggered_id == "expand-all":
return "*"
if ctx.triggered_id =="collapse-all":
return []
return dash.no_update
|
@Godisemo - quick update. Haven't forgotten about this PR. Just getting caught up after the holidays and will get back to this soon. Thanks for your patience 🙂 |
I think the prop names & behaviors are good. The
Also: even leaf nodes get listed in |
src/ts/components/core/Tree.tsx
Outdated
transform: expanded | ||
? "rotate(180deg)" | ||
: "rotate(0deg)", |
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 know this is how the Mantine docs do it, but I find this very confusing. The standard behavior, that you can see even right here in GitHub, is that a collapsed node has the chevron pointing to the right, and an expanded node has it pointing down.
Which I think would be this:
transform: expanded | |
? "rotate(180deg)" | |
: "rotate(0deg)", | |
transform: expanded | |
? "rotate(0deg)" | |
: "rotate(270deg)", |
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 catch - I found those chevrons confusing but couldn't say why. lol
Part of the bigger issue with wrapping this component for Dash is how to customize things like this chevron, and formatting the Tree to include other components like icons. Even this forum topic had a debate on whether the chevron should be to the right or left of the text....
If you were creating an app using React and Mantine, then it's easy to customize all of these things. But how can we design an API for Dash to allow this flexibility?
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.
Right, that's always a challenge when going from a relatively low-level interface like Mantine to a higher-level interface like Dash components. Generally I'd say you don't design for all the flexibility Mantine gives you; you make smart choices for the defaults and then add flexibility where a significant number of users will want it.
And "which side are the chevrons on" might be just such a case - personally I prefer them on the left, and given that that's where Mac, Windows, and GH all put them I think that would be a solid default, but since others like the right side we could certainly add a prop like chevronSide = "left" | "right" | "none"
One note though about putting them on the left: leaf nodes don't get a chevron, but we should probably add blank space as if they did, otherwise leaves won't look like they're really inside their parent folders.
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.
One thought I had on this when making the PR was to add properties for leftSection and rightSection (as for other dmc components) which could then be set to a DashIconify icon which the user can specify themselves. What deterred me from that though was how to deal with open/closed state. Should it be leftSectionOpen and leftSectionClosed or should leftSection optionally take a dict with keys “open” and “closed”?. If you think this would be a way to go and if you have suggestions I’d be happy to update the PR. One additional benefit of having the user be able to specify separate icons instead of rotating using css would be that you can have e.g + and - icons instead of chevrons or triangles.
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.
Good point about the icon not necessarily being a chevron - so chevronSide
would be limiting (also maybe not the most well-known word!). But do you ever see users wanting changeable icons on both sides? That feels weird to me, seems like the icon that changes when the node is opened or closed will either be on the left or right, and if you want an unchanging icon on the other side you can presumably put that in the label
?
If we agree on that, then we can have maybe expandIconSide = "left" | "right" | "none"
?
Then to the question of changing the icons: The advantage of a CSS rotation is you can animate it (which means we should use -90deg
rather than 270deg
as I had suggested above). Would animation still work if you specified the same icon with different CSS? Something like:
collapsedIcon=DashIconify(icon="tabler:chevron-down", style={"transform": "rotate(-90deg)", "transition": "transform 0.5s"})
expandedIcon=DashIconify(icon="tabler:chevron-down", style={"transform": "rotate(0deg)", "transition": "transform 0.5s"})
If that preserves the animation, it seems like a good compromise, covering the vast majority of use cases without bloating the API too much.
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.
Changing the icons based on state, could be done like the Checkbox
with the checked
or indeterminate
states:
https://www.dash-mantine-components.com/components/checkbox#change-icons
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.
Good point about the icon not necessarily being a chevron - so
chevronSide
would be limiting (also maybe not the most well-known word!). But do you ever see users wanting changeable icons on both sides? That feels weird to me, seems like the icon that changes when the node is opened or closed will either be on the left or right, and if you want an unchanging icon on the other side you can presumably put that in thelabel
?If we agree on that, then we can have maybe
expandIconSide = "left" | "right" | "none"
?Then to the question of changing the icons: The advantage of a CSS rotation is you can animate it (which means we should use
-90deg
rather than270deg
as I had suggested above). Would animation still work if you specified the same icon with different CSS? Something like:collapsedIcon=DashIconify(icon="tabler:chevron-down", style={"transform": "rotate(-90deg)", "transition": "transform 0.5s"}) expandedIcon=DashIconify(icon="tabler:chevron-down", style={"transform": "rotate(0deg)", "transition": "transform 0.5s"})
If that preserves the animation, it seems like a good compromise, covering the vast majority of use cases without bloating the API too much.
Animation does indeed still work when specifying the same icon. I have updated the PR to include this functionality.
The functionality is indeed a bit quirky in upstream Mantine, but as you say I agree we shouldn't do anything about it here. |
I have updated the PR to include three new properties |
The changes look great! 🎉 Here's a draft of the docs: snehilvj/dmc-docs#147 For the
I only found one issue: Not sure what's causing this error in the console:
@Godisemo are you familiar with Dash testing? If so, you can check out the tests for other components in the |
I also noticed this, but I concluded it comes from the code highlighting component.
I will try to add some unit tests. |
Update: The console error is not coming from the import dash_mantine_components as dmc
from dash import Dash, _dash_renderer
_dash_renderer._set_react_version("18.2.0")
app = Dash()
data2 = [
{
"value": "src",
"label": "src",
"children": [
{"value": "src/components", "label": "components"},
{"value": "src/hooks", "label": "hooks"},
],
},
{"value": "package.json", "label": "package.json"},
]
app.layout = dmc.MantineProvider(dmc.Tree(data=data))
if __name__ == "__main__":
app.run(debug=True)
|
@Godisemo |
@AnnMarieW It might be possible to do, but I think it would require making some sort of custom leaf renderer dmc component. |
OK, that makes sense. That could be a path forward for a future PR if this is a popular request. |
@AnnMarieW I have added a few tests. Let me know if there is something else that should be updated before this PR can be accepted. |
@Godisemo @alexcjohnson - do you have any more comments before merging? |
tests/test_tree.py
Outdated
|
||
@app.callback(Output("output-checked", "children"), Input("tree", "checked")) | ||
def update_output_checked(checked): | ||
return str(checked) |
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.
When I was playing with this feature I found the ordering of lists returned by the tree to be confusing, as it depends in odd ways on the order of operations. So although the tests as you've written them are robust for the current version of Mantine, I wouldn't be surprised if in the future they change this. Also the tests themselves would be easier to read if these fields were sorted - which as you've constructed it is also the order on screen.
So I'd suggest changing this line to:
return str(checked) | |
return str(sorted(checked) if checked else checked) |
(the if checked else checked
just lets None
through without an error, not sure if that's necessary) and the same for expanded
and selected
(not data
of course 😄) and adjusting the test assertions to match.
And perhaps worth mentioning this inconsistent order in the docs?
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.
💃 This looks great to me. Lovely tests 😻 Just one suggestion but that's not blocking.
Good point regarding the tests, I’d be happy to update them before merging this. |
Updated the tests now. @AnnMarieW Thank you for adding the docs for this. Will you also mention what Alex pointed out about the |
Yes, I'll update the docs to include this. Thanks so much for adding this component and |
Add tree component.
Example app below