-
Notifications
You must be signed in to change notification settings - Fork 11
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
⚙️ Fixes dependency tree-view #186
Conversation
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.
Thank you for your contributions. However, the current dependency tree-view is not resolving the issue properly; it is also causing some new ones:
It is not seamless, and it requires multiple clicks to open the dependency tree view. I tried it locally, and as a user, I find it more difficult than the previous one. Sometimes it gets auto-zoomed, and it also requires multiple clicks to open the graph. You added some more info, which is good, but the way it's displayed makes the tree less user-friendly. I suggest making it a bit similar to the previous version or just solving the problems we had previously instead of changing the styles entirely. I think this approach might enable us to achieve better results rather than revamping it all.
As you can see in the video below, sometimes it requires multiple clicks and gets auto-zoomed when clicked on the last node of the dependency tree. I believe this needs to be fixed :
StatWrap.2024-03-15.09-52-24.mp4
Also, the extra info that you added here like "asset type" is nice, but I am not sure if we need that. If we really need it, then I think it can be displayed in a much better way.
Hi @Abhijay007 Actually, what you are facing with Auto-zooming is because this graph has enabled zooming on scroll/double-click so when you are not acquainted with the scroll and zoom mechanism, you might make mistakes zooming it in, for that, I'd like to tell you how exactly the zoom/move works with this one. User Interactions
These probably sum up the zoom/move interactions, if I missed any I'd surely let you all know. I've checked myself that am not getting any unwanted zoom/move other than these. And, for the multiple clicks to open dependency tree - I believe you're talking about it when we load this tree while we come from some other page, or when we initially render it, for which I'd recommend you to not click multiple times, and just wait for it to render, actually this is the what I faced with the earlier tree-view as well, the data being very complex, that too used to take a bit of time to load completely, and I believe their speeds are comparable. AttachmentsCurrent Treeview 2424a333-70af-4a37-8a1b-c9d5384b8613.mp4New Treeview Demonstration bbd749a1-54c9-48c5-877b-25e2ab7747d1.mp4The addition of "asset type" can be discussed with how the maintainers would want it to look, this is just the UI and view that looked decent to me, and the font family that looked pretty cool with this tree view, we can obviously alter those things as I get some input. The one great thing with this package is that it offers quite decent control over the UI and functionality. @lrasmus Please take a look at it and suggest any changes to make it better. |
Thank you @AdiAkhileshSingh15 for the proposed fix, and @Abhijay007 for your feedback! I'd like to run this by a few of the other team members to get their input as well. |
e2f8f3b
to
35f2dcb
Compare
@AdiAkhileshSingh15 - per the comment made, we would love to play around with the changes you've made more. We would like some hands on time to test ourselves, so my request is to have your changes available in the D3 version of the tree. We are not going to fully enable it in the main version yet, but this is a great contribution, and we would love to have it in the code base to work with more. Thanks! |
Thanks for your response @lrasmus |
@lrasmus Please check, I've updated the branch |
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.
Thank you! Changes look good, we are excited to try this out more.
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.
We would love to have this available to try out. We had created 2 versions of the tree and graph components - one for D3 and one for ECharts. Because this uses react-d3-tree, could you please move these changes to the file: https://github.com/StatTag/StatWrap/blob/master/app/components/Workflow/DependencyTree/DependencyTreeD3.js?
Changes Summary
This PR aims to make the Dependency Tree View tidy and render it free from unwanted UI glitches. As discussed in the issue #182, it was not properly retracting for open branches.
After confirming that the issue was with the tree rendering package, which was unable to deal with the complex data generated by StatWrap, and not the
data
object itself, I explored some npm packages which MIT (or Apache2) license, namely @carbon/charts-react, gojs & react-d3-tree, etc.The first one didn't offer much control over the UI for tree-view, gojs didn't quite fit into how we render the data and the code format, so, I went with this d3-tree which worked quite well and fit better than the earlier one, which I later came to know while noticing no changes in the package.json, that this was earlier used to render the dependency tree as well :))
Which left me trying to render the old one, but it was not properly setup so, the view was cluttered at a corner.
I'd thus like to clean that up, as now it's of no use, if you allow.
Styling and color-scheme used was upto my color-sense which I don't think is good enough :) The prop values are set by trial and testing the UI, so, I'd suggest you to modify these with what looks better to you.
Attachments
Initial view for depth 2.
When the app branch is closed with backend branch open.
When the backend branch is closed with app branch open.
ScreenRec isn't currently working, transition is looking quite decent. Will try to attach it as well though, later.
Related Issue 🐛
Closes #182
Checklist ✅
Reviewer
@lrasmus