feat(topology): Enable saving current topology graph model #4690

jeff-phillips-18 · 2020-08-13T19:19:44Z

What:
This PR is aimed at allowing adopters to track the current layout of nodes so that it can be saved and restored.

Provides API for toModel for the current Controller/GraphElements.
Keeps track of nodes that have had their position set, do not reset positions on layout.
Adds events on individual node moves, and overall event for all node moves (debouncing individual node moves)

/cc @christianvogt

patternfly-build · 2020-08-13T19:28:36Z

PF4 preview: https://patternfly-react-pr-4690.surge.sh
A11y report: https://patternfly-react-pr-4690-coverage.surge.sh

ddonahue007

LGTM

christianvogt · 2020-08-18T16:02:09Z

@jeff-phillips-18 i'll have a look at this today or tomorrow

christianvogt · 2020-08-18T20:50:45Z

packages/react-topology/src/types.ts

@@ -126,6 +127,7 @@ export interface Node<E extends NodeModel = NodeModel, D = any> extends GraphEle
  setBounds(bounds: Rect): void;
  getPosition(): Point;
  setPosition(location: Point): void;
+  clearPosition(): void;


I think we can remove clearPosition.
All other comments are related to trying to remove this function.

christianvogt · 2020-08-18T20:51:39Z

packages/react-topology/src/layouts/BaseLayout.ts

@@ -439,7 +439,7 @@ class BaseLayout implements Layout {
    this.edges = this.getLinks(this.graph.getEdges());

    // initialize new node positions
-    this.initializeNodePositions(newNodes, this.graph, initialRun);
+    this.initializeNodePositions(this.nodes, this.graph);


Let's keep initialRun as a param to force layout on the nodes.

christianvogt · 2020-08-18T20:52:24Z

packages/react-topology/src/layouts/BaseLayout.ts

+      if (!node.element.isPositioned()) {
        node.setPosition(cx, cy);
+      } else {
+        node.setFixed(true);
      }


Suggested change

if (!node.element.isPositioned()) {

node.setPosition(cx, cy);

} else {

node.setFixed(true);

}

if (force || !node.element.isPositioned()) {

node.setPosition(cx, cy);

} else if (!force) {

node.setFixed(true);

}

christianvogt · 2020-08-18T20:55:41Z

packages/react-topology/src/elements/BaseNode.ts

+    try {
+      this.getController().fireEvent(NODE_POSITIONED_EVENT, { node: this });
+      // eslint-disable-next-line no-empty
+    } catch (e) {}


A bit concerned that this can result in a lot of event notifications during the layout which may impact performance.

I wonder if we should consider a single event notification that's debounced that can announce model changed. This isn't really ideal either.

christianvogt · 2020-08-18T20:56:51Z

packages/react-topology/src/elements/BaseGraph.ts

+    try {
+      this.getController()
+        .getElements()
+        .filter(e => isNode(e))
+        .forEach(n => {
+          (n as Node).clearPosition();
+        });
+      // eslint-disable-next-line no-empty
+    } catch (e) {}


We can remove this if we can remove clearPosition altogether.

christianvogt · 2020-08-19T18:50:29Z

packages/react-topology/src/elements/BaseEdge.ts

+      source: this.getSource() ? this.getSource().getId() : '',
+      target: this.getTarget() ? this.getTarget().getId() : '',


I think we should have these as undefined:

Suggested change

source: this.getSource() ? this.getSource().getId() : '',

target: this.getTarget() ? this.getTarget().getId() : '',

source: this.getSource() ? this.getSource().getId() : undefined,

target: this.getTarget() ? this.getTarget().getId() : undefined,

christianvogt · 2020-08-19T18:53:57Z

packages/react-topology/src/elements/BaseNode.ts

+      x: this.getPosition().x,
+      y: this.getPosition().y,


Only include the position if this.positioned === true

christianvogt · 2020-08-19T18:56:06Z

packages/react-topology/src/types.ts

@@ -171,6 +173,7 @@ export interface Graph<E extends GraphModel = GraphModel, D = any> extends Graph
  getLayout(): string | undefined;
  setLayout(type: string | undefined): void;
  layout(): void;
+  stopLayout(): void;


I'd prefer to not add this function to the graph api.
They can always do graph.getLayout()?.stopLayout()

christianvogt · 2020-08-19T18:56:38Z

packages/react-topology/src/types.ts

@@ -9,6 +9,7 @@ export type PointTuple = [number, number];

 export interface Layout {
  layout(): void;
+  stopLayout(): void;


Call it stop(): void ? Since this is a function of the layout itself.

christianvogt · 2020-08-19T19:00:07Z

packages/react-topology/src/Visualization.ts

+    // If not merging, clear out the old elements
+    if (!merge) {
+      this.elements = {};
+    }


We may want to go through the routine of destroying the element for the sake of consistency.

codecov-commenter · 2020-08-21T16:08:10Z

Codecov Report

Merging #4690 into master will decrease coverage by 0.08%.
The diff coverage is 28.20%.

@@            Coverage Diff             @@
##           master    #4690      +/-   ##
==========================================
- Coverage   52.52%   52.44%   -0.09%     
==========================================
  Files         514      514              
  Lines        8974     9006      +32     
  Branches     3266     3277      +11     
==========================================
+ Hits         4714     4723       +9     
- Misses       3673     3693      +20     
- Partials      587      590       +3

Flag	Coverage Δ
#patternfly4	`52.44% <28.20%> (-0.09%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/react-topology/src/elements/BaseEdge.ts	`1.51% <0.00%> (-0.10%)`	⬇️
...ackages/react-topology/src/elements/BaseElement.ts	`26.41% <0.00%> (+1.41%)`	⬆️
packages/react-topology/src/elements/BaseGraph.ts	`86.44% <0.00%> (-2.26%)`	⬇️
packages/react-topology/src/Visualization.ts	`34.37% <27.77%> (-2.70%)`	⬇️
packages/react-topology/src/elements/BaseNode.ts	`32.53% <45.45%> (+1.50%)`	⬆️
packages/react-topology/src/types.ts	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1697af5...278c546. Read the comment docs.

christianvogt · 2020-08-21T16:56:29Z

lgtm

jeff-phillips-18 · 2020-08-21T16:58:25Z

@redallen @ddonahue007 Please take another look.

redallen

Still LGTM!

ddonahue007

LGTM

patternfly-build · 2020-08-27T14:21:47Z

Your changes have been released in:

@patternfly/react-docs@5.8.23
demo-app-ts@4.36.0
@patternfly/react-integration@4.36.0
@patternfly/react-topology@4.5.0

Thanks for your contribution! 🎉

jeff-phillips-18 requested review from wojta and redallen August 13, 2020 19:19

jeff-phillips-18 force-pushed the serialize-layout branch from e3f653e to 92c02ca Compare August 17, 2020 11:06

jeff-phillips-18 requested review from ddonahue007 and kmcfaul August 18, 2020 14:04

redallen previously approved these changes Aug 18, 2020

View reviewed changes

ddonahue007 previously approved these changes Aug 18, 2020

View reviewed changes

christianvogt reviewed Aug 18, 2020

View reviewed changes

jeff-phillips-18 dismissed stale reviews from ddonahue007 and redallen via 0a634a6 August 19, 2020 17:23

jeff-phillips-18 force-pushed the serialize-layout branch from 92c02ca to 0a634a6 Compare August 19, 2020 17:23

christianvogt reviewed Aug 19, 2020

View reviewed changes

jeff-phillips-18 force-pushed the serialize-layout branch 4 times, most recently from e2aee28 to 4d8da99 Compare August 21, 2020 14:32

feat(topology): Enable saving current topology graph model

278c546

jeff-phillips-18 force-pushed the serialize-layout branch from 4d8da99 to 278c546 Compare August 21, 2020 15:12

redallen approved these changes Aug 21, 2020

View reviewed changes

ddonahue007 approved these changes Aug 21, 2020

View reviewed changes

dlabrecq approved these changes Aug 25, 2020

View reviewed changes

redallen approved these changes Aug 27, 2020

View reviewed changes

redallen merged commit 2c4b25c into patternfly:master Aug 27, 2020

jeff-phillips-18 deleted the serialize-layout branch November 11, 2021 14:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(topology): Enable saving current topology graph model #4690

feat(topology): Enable saving current topology graph model #4690

jeff-phillips-18 commented Aug 13, 2020

patternfly-build commented Aug 13, 2020 •

edited

Loading

ddonahue007 left a comment

christianvogt commented Aug 18, 2020

christianvogt Aug 18, 2020

christianvogt Aug 18, 2020

christianvogt Aug 18, 2020

christianvogt Aug 18, 2020

christianvogt Aug 18, 2020

christianvogt Aug 19, 2020

christianvogt Aug 19, 2020

christianvogt Aug 19, 2020

christianvogt Aug 19, 2020

christianvogt Aug 19, 2020

codecov-commenter commented Aug 21, 2020

christianvogt commented Aug 21, 2020

jeff-phillips-18 commented Aug 21, 2020

redallen left a comment

ddonahue007 left a comment

patternfly-build commented Aug 27, 2020

		source: this.getSource() ? this.getSource().getId() : '',
		target: this.getTarget() ? this.getTarget().getId() : '',

feat(topology): Enable saving current topology graph model #4690

feat(topology): Enable saving current topology graph model #4690

Conversation

jeff-phillips-18 commented Aug 13, 2020

patternfly-build commented Aug 13, 2020 • edited Loading

ddonahue007 left a comment

Choose a reason for hiding this comment

christianvogt commented Aug 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Aug 21, 2020

Codecov Report

christianvogt commented Aug 21, 2020

jeff-phillips-18 commented Aug 21, 2020

redallen left a comment

Choose a reason for hiding this comment

ddonahue007 left a comment

Choose a reason for hiding this comment

patternfly-build commented Aug 27, 2020

patternfly-build commented Aug 13, 2020 •

edited

Loading