Flowchart Orientation #2251
Flowchart Orientation #2251
Conversation
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
Signed-off-by: rashidakanchwala <[email protected]>
There was a problem hiding this comment.
Awesome work @rashidakanchwala, tested it manually and looks great!
| ariaLabel="Change flowchart orientation" | ||
| className={'pipeline-menu-button--orientation'} | ||
| dataTest={'sidebar-flowchart-orientation-btn'} | ||
| icon={orientation === 'vertical' ? TopBottomIcon : LeftRightIcon} |
There was a problem hiding this comment.
should this be opposite ? Like when vertical we show LeftRightIcon to change, similar to expand/collapse pipelines ?
There was a problem hiding this comment.
this is a very good point. i am not sure; once @stephkaiser is back; let's ask her.
Signed-off-by: rashidakanchwala <[email protected]>
; Signed-off-by: rashidakanchwala <[email protected]>
| @@ -9,26 +9,22 @@ | |||
| import { Constraint, Operator, Strength } from 'kiwi.js'; | |||
|
|
|||
| /** | |||
| * Layout constraint in Y for separating rows | |||
| * Layout constraint in for separating rows | |||
There was a problem hiding this comment.
| * Layout constraint in for separating rows | |
| * Layout constraint for separating rows |
| Strength.required | ||
| ), | ||
| }; | ||
|
|
||
| /** | ||
| * Layout constraint in Y for separating layers | ||
| * Layout constraint in for separating layers |
There was a problem hiding this comment.
| * Layout constraint in for separating layers | |
| * Layout constraint for separating layers |
| @@ -39,16 +35,15 @@ export const layerConstraint = { | |||
| }; | |||
|
|
|||
| /** | |||
| * Layout constraint in X for minimising distance from source to target for straight edges | |||
| * Layout constraint in for minimising distance from source to target for straight edges | |||
There was a problem hiding this comment.
| * Layout constraint in for minimising distance from source to target for straight edges | |
| * Layout constraint for minimising distance from source to target for straight edges |
| @@ -61,37 +56,39 @@ export const parallelConstraint = { | |||
| }; | |||
|
|
|||
| /** | |||
| * Crossing constraint in X for minimising edge crossings | |||
| * Crossing constraint in for minimising edge crossings | |||
There was a problem hiding this comment.
| * Crossing constraint in for minimising edge crossings | |
| * Crossing constraint for minimising edge crossings |
| }, | ||
| }; | ||
|
|
||
| /** | ||
| * Layout constraint in X for minimum node separation | ||
| * Layout constraint in for minimum node separation |
There was a problem hiding this comment.
| * Layout constraint in for minimum node separation | |
| * Layout constraint for minimum node separation |
| node.x = 0; | ||
| node.y = 0; | ||
| node[coordPrimary] = 0; | ||
| node[coordSecondary] = 0; | ||
| } | ||
|
|
||
| // Constants used by constraints | ||
| const constants = { |
There was a problem hiding this comment.
[Nit] Though not introduced in this PR, constants variable name does not feel proper. Something like constraintDefaults ?
| }; | ||
|
|
||
| /** | ||
| * Creates row constraints for the given edges. | ||
| * @param {Array} edges The input edges | ||
| * @returns {Array} The constraints | ||
| */ | ||
| const createRowConstraints = (edges) => | ||
| const createRowConstraints = (edges, constants) => |
There was a problem hiding this comment.
Can you please add some comments on what does Creates row constraints for the given edges. means ?
| @@ -106,14 +107,24 @@ export const routing = ({ | |||
|
|
|||
| const offsetX = Math.min(spaceX, nodeGap * 0.5); | |||
|
|
|||
| let sourceX, sourceY, targetX, targetY; | |||
|
|
|||
| //TODO: Need to do this for horizontal orientation as well. | |||
| @@ -57,20 +57,22 @@ export const solveStrict = (constraints, constants) => { | |||
| }; | |||
|
|
|||
| for (const constraint of constraints) { | |||
| addVariable(constraint.a, constraint.base.property); | |||
| addVariable(constraint.b, constraint.base.property); | |||
| const property = constraint.property || constraint.base.property; | |||
There was a problem hiding this comment.
I do see constraint has property everywhere. Is there a place where we have property on base and not constraint ?
Also, in simple terms what is a constraint , base, property ?
|
Hi @rashidakanchwala , I did another pass at the PR and left few comments. The code looks good and seems quite complex. It would be great if you can add some comments/context (mostly related to graph constraints rows, layers, edges, cross constraints). I could understand most parts but the code at Thank you |
Description
Resolves #2159
Development notes
For first PR review, pls QA it and look through the code. I will add dev notes when i am back from PTO
QA notes
Checklist
RELEASE.mdfile