Flowchart Orientation #2251

rashidakanchwala · 2025-01-28T11:46:15Z

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

Read the contributing guidelines
Opened this PR as a 'Draft Pull Request' if it is work-in-progress
Updated the documentation to reflect the code changes
Added new entries to the RELEASE.md file
Added tests to cover my changes

Signed-off-by: rashidakanchwala <[email protected]>

astrojuanlu

I gave this a quick run using the demo project and looks like it works! 😄

SajidAlamQB

Awesome work @rashidakanchwala, tested it manually and looks great!

ravi-kumar-pilla · 2025-02-07T21:43:35Z

src/components/flowchart-primary-toolbar/flowchart-primary-toolbar.js

+          ariaLabel="Change flowchart orientation"
+          className={'pipeline-menu-button--orientation'}
+          dataTest={'sidebar-flowchart-orientation-btn'}
+          icon={orientation === 'vertical' ? TopBottomIcon : LeftRightIcon}


should this be opposite ? Like when vertical we show LeftRightIcon to change, similar to expand/collapse pipelines ?

this is a very good point. i am not sure; once @stephkaiser is back; let's ask her.

src/components/flowchart/draw.js

src/selectors/layers.js

src/utils/graph/common.js

Signed-off-by: rashidakanchwala <[email protected]>

; Signed-off-by: rashidakanchwala <[email protected]>

ravi-kumar-pilla · 2025-02-11T22:11:53Z

src/utils/graph/constraints.js

@@ -9,26 +9,22 @@
 import { Constraint, Operator, Strength } from 'kiwi.js';

 /**
- * Layout constraint in Y for separating rows
+ * Layout constraint in  for separating rows


Suggested change

* Layout constraint in for separating rows

* Layout constraint for separating rows

ravi-kumar-pilla · 2025-02-11T22:12:48Z

src/utils/graph/constraints.js

      Strength.required
    ),
 };

 /**
- * Layout constraint in Y for separating layers
+ * Layout constraint in for separating layers


Suggested change

* Layout constraint in for separating layers

* Layout constraint for separating layers

ravi-kumar-pilla · 2025-02-11T22:13:22Z

src/utils/graph/constraints.js

@@ -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


Suggested change

* 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

ravi-kumar-pilla · 2025-02-11T22:14:17Z

src/utils/graph/constraints.js

@@ -61,37 +56,39 @@ export const parallelConstraint = {
 };

 /**
- * Crossing constraint in X for minimising edge crossings
+ * Crossing constraint in for minimising edge crossings


Suggested change

* Crossing constraint in for minimising edge crossings

* Crossing constraint for minimising edge crossings

ravi-kumar-pilla · 2025-02-11T22:14:59Z

src/utils/graph/constraints.js

  },
 };

 /**
- * Layout constraint in X for minimum node separation
+ * Layout constraint in for minimum node separation


Suggested change

* Layout constraint in for minimum node separation

* Layout constraint for minimum node separation

ravi-kumar-pilla · 2025-02-11T22:17:59Z

src/utils/graph/layout.js

-    node.x = 0;
-    node.y = 0;
+    node[coordPrimary] = 0;
+    node[coordSecondary] = 0;
  }

  // Constants used by constraints
  const constants = {


[Nit] Though not introduced in this PR, constants variable name does not feel proper. Something like constraintDefaults ?

ravi-kumar-pilla · 2025-02-11T22:23:12Z

src/utils/graph/layout.js

 };

 /**
 * Creates row constraints for the given edges.
 * @param {Array} edges The input edges
 * @returns {Array} The constraints
 */
-const createRowConstraints = (edges) =>
+const createRowConstraints = (edges, constants) =>


Can you please add some comments on what does Creates row constraints for the given edges. means ?

ravi-kumar-pilla · 2025-02-11T22:32:40Z

src/utils/graph/routing.js

@@ -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.


ravi-kumar-pilla · 2025-02-11T22:34:17Z

src/utils/graph/solver.js

@@ -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;


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 ?

ravi-kumar-pilla · 2025-02-11T22:38:36Z

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 src/utils/graph/* needs another pass :)

Thank you

jitu5 · 2025-02-12T10:09:04Z

src/components/flowchart/draw.js

+  const { chartSize, layers, orientation } = this.props;
+
+  const layerNamePosition =
+    orientation === 'vertical' ? chartSize.sidebarWidth || 0 : 100 || 0;


Nit:

Suggested change

orientation === 'vertical' ? chartSize.sidebarWidth || 0 : 100 || 0;

orientation === 'vertical' ? chartSize.sidebarWidth || 0 : 100;

jitu5 · 2025-02-17T12:58:56Z

@rashidakanchwala When I was testing it locally I observed,

In horizontal orientation pipeline slicing is not working
After selecting horizontal orientation and if I reload the page, right now its not retaining the horizontal orientation.

Is it expected for this PR ?

rashidakanchwala · 2025-02-17T14:15:41Z

@rashidakanchwala When I was testing it locally I observed,

In horizontal orientation pipeline slicing is not working

After selecting horizontal orientation and if I reload the page, right now its not retaining the horizontal orientation.

Is it expected for this PR ?

Good catch, let me investgatee
Yes, i am not storing it in localStorage. let me add that now. thanks for pointing.

jitu5 · 2025-02-18T10:19:04Z

In horizontal orientation pipeline slicing is not working

@rashidakanchwala Pipeline slicing is working as expected.

Signed-off-by: rashidakanchwala <[email protected]>

stephkaiser · 2025-02-19T11:38:34Z

Awesome work @rashidakanchwala! some design QA notes below:

Button tooltip - lowercase 'O' for 'Orientation' - should be: 'Change orientation'

Reset view button doesn't show the full flowchart in both horizontal and landscape orientation (its currently broken on demo site as well)

Order of the orientation button in the actions bar -

Transition animation of the layers labels in horizontal orientation looks odd (see the 'intermediate' text overlaps with 'primary')
Layers highlights on hover are slightly off and not aligned with the layer labels
In regards to @ravi-kumar-pilla's comment on button icon behaviour, lets keep it for now. It is inconsistent with the expand/collapse button but they are different also - Expand/collapse changes to a different button, while orientation button indicates which is the current orientation. Both make sense to me even though they are not consistent with each other. I'll review and discuss this with other designers to see what they think but for now I think we can keep it as it is.
Lines are looking really weird, can we improve this?

Thank you!!

rashidakanchwala · 2025-02-19T13:32:06Z

Will do.
I attempted to fix this, but as you noticed, it affects both horizontal and vertical orientations. This has been an existing issue for a long time. A trade-off was made here, as the best fit sometimes results in a very wide and short visual. We should address this as a separate issue since it’s not specific to horizontal orientation—it has existed since the Kedro-Viz flowchart algorithm was written.
Will do.
I'll look into this fix.
I tried fixing this issue and time-boxed it, but I wasn’t able to get it working. I can spend some more time on it.
Makes sense.
This trade-off was made for optimisation. The lines (that go over multiple layers) behave unpredictably in vertical orientation as well. I had raised this issue in the last PS, and we had decided to revisit this later since it took me a long time to get it to this stage, and further improvements would require a lot of time.

@stephkaiser , pls see my responses below.
cc - @astrojuanlu

stephkaiser · 2025-02-19T15:05:50Z

sounds good, lets tackle in a separate issue

7 . understood, lets revisit this in the future then

thank you @rashidakanchwala !!

Signed-off-by: rashidakanchwala <[email protected]>

rashidakanchwala · 2025-02-19T15:57:54Z

@stephkaiser

I've fixed points 1, 3, and 5. I also attempted to address the transition issue (point 4) , but I'd like to understand from you how the transitions should behave since they affect both horizontal and vertical orientations.

This seems to be an existing issue, so we can also tackle it in a separate PR if needed. I tried removing transitions, but it didn’t fully resolve the problem. Let me know your thoughts!

stephkaiser · 2025-02-19T17:47:46Z

@stephkaiser

I've fixed points 1, 3, and 5. I also attempted to address the transition issue (point 4) , but I'd like to understand from you how the transitions should behave since they affect both horizontal and vertical orientations.

This seems to be an existing issue, so we can also tackle it in a separate PR if needed. I tried removing transitions, but it didn’t fully resolve the problem. Let me know your thoughts!

It looks to me like there is a delay when the layer labels change, would shortening the animation of the layer labels solve this? we can jump on a quick call tomorrow if thats useful

Signed-off-by: rashidakanchwala <[email protected]>

WIP

8abae8d

Signed-off-by: rashidakanchwala <[email protected]>

rashidakanchwala changed the base branch from new-flowchart-rtl to main January 28, 2025 11:49

rashidakanchwala added 8 commits January 28, 2025 14:11

graph calc done

2cfb895

Signed-off-by: rashidakanchwala <[email protected]>

WIP

dc8aae6

Signed-off-by: rashidakanchwala <[email protected]>

fixed layers

abcb7d5

Signed-off-by: rashidakanchwala <[email protected]>

test graph utils done

df900b1

Signed-off-by: rashidakanchwala <[email protected]>

component tests passing

e74e3fe

Signed-off-by: rashidakanchwala <[email protected]>

some tests

e55e8c2

Signed-off-by: rashidakanchwala <[email protected]>

added new icon

1b44896

Signed-off-by: rashidakanchwala <[email protected]>

icon

5955b99

Signed-off-by: rashidakanchwala <[email protected]>

rashidakanchwala marked this pull request as ready for review February 5, 2025 17:12

rashidakanchwala requested a review from jitu5 as a code owner February 5, 2025 17:12

Merge branch 'main' into flowchart-orientation

e0b7677

rashidakanchwala requested review from ravi-kumar-pilla and SajidAlamQB February 5, 2025 17:12

astrojuanlu reviewed Feb 6, 2025

View reviewed changes

SajidAlamQB approved these changes Feb 7, 2025

View reviewed changes