-
Notifications
You must be signed in to change notification settings - Fork 3.2k
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
fix(ui): Tiny modal DAG tweaks. Fixes #4039 #4043
Conversation
|
||
private get vgap() { | ||
return this.nodeSize; | ||
private get gap() { |
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.
using a single gap value makes layout more predictable
@@ -568,6 +563,12 @@ export class WorkflowDag extends React.Component<WorkflowDagProps, WorkflowDagRe | |||
} | |||
|
|||
private layoutGraph(nodes: string[], edges: Edge[]) { | |||
const hash = {scale: this.scale, nodeCount: nodes.length, nodesToDisplay: this.state.nodesToDisplay}; |
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.
move this check here so both graphs benefit
edgesep: this.gap, | ||
nodesep: this.gap, | ||
rankdir: this.state.horizontal ? 'LR' : 'TB', | ||
ranksep: 50 / this.scale | ||
ranksep: this.gap |
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.
Does this change how the graph looks? Could you add a screenshot?
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 makes it more compact - moving from 50px sep to 32px sep (default zoom)
Sorry I know this is still in draft mode... jumped the gun on the review |
Checklist:
"fix(controller): Updates such and such. Fixes #1234"
.I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.Fixes #4039
Related to #3595 (#3967)