-
Notifications
You must be signed in to change notification settings - Fork 317
Display Graph #377
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
Display Graph #377
Conversation
web/client/package.json
Outdated
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.
dagre's deprecated
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.
have you tried visjs?
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 was in example , so I used it just to make it work for now.
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.
let's not use dagre, even for an example
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.
so I used https://github.com/kieler/elkjs, since we only need a graph layout engine but vis-network seems like a draw library
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.
ok
47fed35 to
e40c78e
Compare
|
For these kind of UI changes, would it be possible to attach a screenshot, or even an animated GIF, of the changes to the PR? |
web/server/api/endpoints/commands.py
Outdated
| @router.get("/dag") | ||
| async def dag( | ||
| context: Context = Depends(get_loaded_context), | ||
| ) -> t.Dict[str, t.Any]: |
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.
| ) -> t.Dict[str, t.Any]: | |
| ) -> t.Dict[str, t.List[str]]: |
| } | ||
|
|
||
| async function load(): Promise<void> { | ||
| setGraph(undefined) |
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.
Why is this necessary?
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.
reset graph object in case getNodesAndEdges fails
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.
What happens when the graph object is reset? Is the effect hook below the only place that is affected?
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 should trigger re-render , so if getNodesAndEdges takes very long time to finish , we may want to show spinner (or skeleton) before new state for graph is set.
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 the graph object is set to undefined, it won't trigger a re-render because the effect hook below returns if graph is null/undefined, right?
|
|
||
| const graph = await getNodesAndEdges({ data, algorithm }) | ||
|
|
||
| if (isFalse(active)) return |
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.
Should this happen before calling getNodesAndEdges()?
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.
our load is async and runs off thread kinda, so if getNodesAndEdges takes too long and we close dag before it finished if (isFalse(active)) return will prevent from changing state after getNodesAndEdges done
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.
Got it. Thank you for the explanation.
Uh oh!
There was an error while loading. Please reload this page.