Expand/collapse Topology Application Groups #3938

jeff-phillips-18 · 2020-01-13T18:51:57Z

Addresses stories:
https://issues.redhat.com/browse/ODC-2459
https://issues.redhat.com/browse/ODC-2548
https://issues.redhat.com/browse/ODC-2460
https://issues.redhat.com/browse/ODC-2623

Fixes:
https://issues.redhat.com/browse/ODC-2420
https://issues.redhat.com/browse/ODC-2780

/cc @serenamarie125 @Veethika @openshift/team-ux-review

christianvogt · 2020-01-13T23:41:45Z

Screenshot looks good. We'll probably want a follow-up where we can size the groupings better based on the content.

christianvogt · 2020-01-14T14:59:18Z

frontend/packages/topology/src/behavior/usePanZoom.tsx

@@ -31,6 +36,9 @@ export const usePanZoom = (zoomExtent: [number, number] = ZOOM_EXTENT): PanZoomR
        if (node) {
          // TODO fix any type
          const $svg = d3.select(node.ownerSVGElement) as any;
+          if (node && node.ownerSVGElement) {
+            node.ownerSVGElement.addEventListener('mousedown', propagatePanZoomMouseEvent);


Make sure to remove this listener in the dispose function.

jeff-phillips-18 · 2020-01-14T18:02:27Z

Updated to show group badge in application group label:

andrewballantyne

Couple minor changes. Nothing game changing. Good cleanup 👍

andrewballantyne · 2020-01-14T23:38:21Z

frontend/packages/dev-console/src/components/topology/Topology.tsx

@@ -220,4 +228,9 @@ const Topology: React.FC<TopologyProps> = ({ data, serviceBinding }) => {
  );
 };

-export default Topology;
+const TopologyStateToProps = (state: RootState) => {


Suggested change

const TopologyStateToProps = (state: RootState) => {

const TopologyStateToProps = (state: RootState): StateProps => {

andrewballantyne · 2020-01-15T00:20:47Z

frontend/packages/dev-console/src/components/topology/components/nodes/Application.tsx

+import ApplicationNode from './ApplicationNode';
+import ApplicationGroup from './ApplicationGroup';
+
+import './ApplicationGroup.scss';


Want to rename this file to match?

Suggested change

import './ApplicationGroup.scss';

import './Application.scss';

andrewballantyne · 2020-01-15T00:31:59Z

frontend/packages/dev-console/src/components/topology/components/nodes/ResourceIcon.tsx

-    let { height } = textSize;
-    const paddingX = height / 2;
-    const paddingY = height / 14;
+    ({ height, width } = textSize);


Huh... I actually didn't know this was a valid syntax... I don't normally destruct into lets. Neat!

andrewballantyne · 2020-01-15T00:45:15Z

frontend/packages/dev-console/src/components/topology/topology-utils.ts

@@ -490,11 +490,15 @@ export const topologyModelFromDataModel = (dataModel: TopologyDataModel): Model
  });

  const groupNodes: NodeModel[] = dataModel.graph.groups.map((d) => {
+    const data: any = dataModel.topology[d.id] || {};


This feels wrong, but I suspect you can't type it any other way.

andrewballantyne · 2020-01-15T00:52:33Z

frontend/packages/topology/src/elements/BaseNode.ts

-    result.setSize(
-      Math.max(result.width, this.bounds.width),
-      Math.max(result.height, this.bounds.height),
-    );
+    // result.setSize(
+    //   Math.max(result.width, this.bounds.width),
+    //   Math.max(result.height, this.bounds.height),
+    // );


Dead code, delete?

andrewballantyne · 2020-01-15T01:07:43Z

I think your CI tests are bugged (they don't appear to be moving for a few hours now). I know the gpc-console one takes almost 2 hours but the others (images/analyze) do not.

/assign

Hopefully your fix push will rekick it.

andrewballantyne · 2020-01-15T01:29:20Z

/kind feature
/kind bug

christianvogt · 2020-01-15T04:10:39Z

changes to the zoom behavior to fix the click through for hiding dropdown menus looks good.

andrewballantyne

/retest

andrewballantyne · 2020-01-15T12:57:33Z

Ah right, Topology package doesn't have a label and I cannot approve that.

/lgtm

andrewballantyne · 2020-01-15T14:59:47Z

/lgtm

jeff-phillips-18 · 2020-01-15T17:44:16Z

/test analyze

andrewballantyne

/lgtm

jeff-phillips-18 · 2020-01-22T18:44:29Z

Removed aggregate edge handling from Visualization, make application defined.

christianvogt

Got this when i created a knative service:

christianvogt · 2020-01-23T21:58:55Z

frontend/packages/topology/stories/91-CollapsibleGroups.stories.tsx

+import '@patternfly/patternfly/patternfly-addons.css';
+
+export default {
+  title: 'Collapsible Groups',


lol ok we should improve that later

christianvogt · 2020-01-23T22:05:45Z

frontend/packages/topology/src/elements/BaseNode.ts

+      const prevCenter = this.getBounds()
+        .getCenter()
+        .clone();
+      this.collapsed = collapsed;
+      this.getBounds().setCenter(prevCenter.x, prevCenter.y);


We should have a TODO with a comment why this is here so we can address it better.

christianvogt · 2020-01-23T22:20:35Z

frontend/packages/topology/src/elements/BaseNode.ts

@@ -65,12 +74,6 @@ export default class BaseNode<E extends NodeModel = NodeModel, D = any> extends
    const { padding } = this.getStyle<GroupStyle>();
    const result = rect.padding(padding);

-    // ensure size is at least the size of the set bounds
-    result.setSize(


This was used by knative service and other groups that don't currently have children.
If you kept this, does it mess up anything? You've already added a guard in the bounds getter.

christianvogt · 2020-01-23T22:24:19Z

frontend/packages/topology/src/behavior/withRemoveConnector.tsx

      >
-        {show && renderRemove(props.element, onRemove)}
+        {show && renderRemove(props.element, onRemove, startPoint, endPoint)}


Maybe default to this behavior instead:

Suggested change

{show && renderRemove(props.element, onRemove, startPoint, endPoint)}

{show && renderRemove(props.element, onRemove, startPoint || props.element.getStartPoint(), endPoint || props.element.getEndPoint())}

christianvogt · 2020-01-23T22:25:36Z

frontend/packages/topology/src/behavior/withRemoveConnector.tsx

+  startPoint?: Point,
+  endPoint?: Point,


Probably useful to make these always present with the defaults being the edge start and end point?

when using withRemoveConnector we don't yet have the edge.

Actually, with the rework, this is no longer needed.

christianvogt · 2020-01-23T22:41:23Z

frontend/packages/dev-console/src/components/topology/components/edges/ConnectsTo.tsx

  return (
    <BaseEdge className={edgeClasses} element={element} {...others}>
-      <EdgeConnectorArrow dragRef={editAccess ? targetDragRef : undefined} edge={element} />
+      {(!element.getSource().isCollapsed() || !element.getTarget().isCollapsed()) && (


Is all this still required now that we have an aggregate edge?

jeff-phillips-18 · 2020-01-24T12:35:03Z

Fixed the empty knative service display

andrewballantyne · 2020-01-24T12:36:55Z

@jeff-phillips-18 Is the intent to show a collapsed Knative when it's starting up?

jeff-phillips-18 · 2020-01-24T12:47:19Z

Yes. It's better than an empty grey box but we still have https://issues.redhat.com/browse/ODC-2437 to address it better in the future.

andrewballantyne · 2020-01-24T13:31:17Z

Yes. It's better than an empty grey box but we still have https://issues.redhat.com/browse/ODC-2437 to address it better in the future.

Hmmm, not sure it is better... just a different kind of misleading 😄

andrewballantyne

/lgtm

This looks fine to me. Christian is still needed for the approval.

christianvogt · 2020-01-24T18:54:48Z

/approve

openshift-ci-robot · 2020-01-24T18:56:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, jeff-phillips-18, serenamarie125

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~frontend/OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

spadgett · 2020-01-24T20:48:04Z

/hold
for merge queue fix #4065

spadgett · 2020-01-25T04:25:14Z

/hold cancel

jeff-phillips-18 requested a review from christianvogt January 13, 2020 18:51

openshift-ci-robot requested review from serenamarie125 and Veethika January 13, 2020 18:51

openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/dev-console Related to dev-console labels Jan 13, 2020

jeff-phillips-18 force-pushed the collapse-groups branch from 6a060db to 018530e Compare January 13, 2020 22:18

jeff-phillips-18 force-pushed the collapse-groups branch from 018530e to fdc6be5 Compare January 14, 2020 12:45

christianvogt reviewed Jan 14, 2020

View reviewed changes

jeff-phillips-18 force-pushed the collapse-groups branch 2 times, most recently from 9ce9d56 to 0b085b8 Compare January 14, 2020 18:00

andrewballantyne suggested changes Jan 15, 2020

View reviewed changes

openshift-ci-robot assigned andrewballantyne Jan 15, 2020

openshift-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. kind/bug Categorizes issue or PR as related to a bug. labels Jan 15, 2020

jeff-phillips-18 force-pushed the collapse-groups branch from 0b085b8 to 34a43f3 Compare January 15, 2020 12:17

andrewballantyne approved these changes Jan 15, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020

jeff-phillips-18 force-pushed the collapse-groups branch from 34a43f3 to 232e5a9 Compare January 15, 2020 14:43

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 15, 2020

andrewballantyne approved these changes Jan 15, 2020

View reviewed changes

jeff-phillips-18 force-pushed the collapse-groups branch from 4bffa5b to 90f425d Compare January 22, 2020 18:43

jeff-phillips-18 force-pushed the collapse-groups branch from 90f425d to 39968d9 Compare January 22, 2020 19:12

jeff-phillips-18 added 7 commits January 23, 2020 09:49

Refactor Topology Layouts to use common base class

0a7d583

Fire an event when graph layout completes

12d0280

Fix to close modals/dropdowns on graph events

1665628

Topology changes to support collapsible groups

c2d660a

Expand/collapse Topology Application Groups

d031b0d

Topology: Collapse/Expand of Knative Services

caee2f6

Topology: Expand/Collapse of Helm Releases

6ddecb5

jeff-phillips-18 force-pushed the collapse-groups branch from 39968d9 to 6ddecb5 Compare January 23, 2020 15:06

jeff-phillips-18 mentioned this pull request Jan 23, 2020

Topology: Fire an event when graph layout completes #4023

Closed

christianvogt reviewed Jan 23, 2020

View reviewed changes

Updates per review comments

864b7db

jeff-phillips-18 force-pushed the collapse-groups branch from 656430b to 864b7db Compare January 24, 2020 12:25

andrewballantyne approved these changes Jan 24, 2020

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2020

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2020

openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2020

openshift-merge-robot merged commit 26a0fc1 into openshift:master Jan 25, 2020

spadgett added this to the v4.4 milestone Jan 27, 2020

	const TopologyStateToProps = (state: RootState) => {
	const TopologyStateToProps = (state: RootState): StateProps => {

	import './ApplicationGroup.scss';
	import './Application.scss';

	{show && renderRemove(props.element, onRemove, startPoint, endPoint)}
	{show && renderRemove(props.element, onRemove, startPoint \|\| props.element.getStartPoint(), endPoint \|\| props.element.getEndPoint())}

Expand/collapse Topology Application Groups #3938

Expand/collapse Topology Application Groups #3938

Conversation

jeff-phillips-18 commented Jan 13, 2020 • edited

christianvogt commented Jan 13, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Jan 14, 2020

andrewballantyne left a 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

andrewballantyne commented Jan 15, 2020

andrewballantyne commented Jan 15, 2020

christianvogt commented Jan 15, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

andrewballantyne commented Jan 15, 2020

andrewballantyne commented Jan 15, 2020

jeff-phillips-18 commented Jan 15, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

jeff-phillips-18 commented Jan 22, 2020

christianvogt left a 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

Choose a reason for hiding this comment

jeff-phillips-18 commented Jan 24, 2020

andrewballantyne commented Jan 24, 2020

jeff-phillips-18 commented Jan 24, 2020

andrewballantyne commented Jan 24, 2020

andrewballantyne left a comment

Choose a reason for hiding this comment

christianvogt commented Jan 24, 2020

openshift-ci-robot commented Jan 24, 2020

spadgett commented Jan 24, 2020

spadgett commented Jan 25, 2020

jeff-phillips-18 commented Jan 13, 2020 •

edited