Remove DagreD3 to make a solid foundation for the graph

[joshri]

Closes #2437

The main goal here was to remove the DagreD3 library and create the graph nodes and paths ourselves. This gives us a lot more flexibility with the graph code. Not only should it be a lot easier to change, but I think it's easier to understand as well (maybe that's just because I wrote it). I also separated out the actual JSX for the nodes for readability - no longer will you have to scroll through 200 lines of node styling to get to the graph code. I'm starting VERY simple here (although I'm definitely close to what we have right now), and the graph is now ready for any new features or redesigns we want to throw at it!

I also made a small change in the computeReady function to distinguish between ReadyType.NotReady and undefined and followed up accordingly.

How was this change implemented?

Some particulars:

1. Online videos recommended NOT using the useRef hook and instead separating the data formation (ReconciliationGraph) and the rendering (DirectedGraph). We are now mapping out two arrays into g (group) elements - one of nodes (into GraphNodes), and one of links (into paths). We had to do a lot of code to get the foreignObject tag into DagreD3, and now we just throw it into JSX! This also allowed me to move the node html to it's own component, which I'm really happy about.

2. No more graph class - this is the only d3 code for the graph (FOR NOW):

  const root = d3.hierarchy(rootNode, (d) => d.children);
  const makeTree = d3
    .tree()
    .nodeSize([
      nodeSize.width + nodeSize.horizontalSeparation,
      nodeSize.height + nodeSize.verticalSeparation,
    ]);
  const tree = makeTree(root);
  const descendants = tree.descendants();
  const links = tree.links();

The rest is made using svg html tags and their attributes in JSX.

3. I learned something very useful about Typescript + styled components:

type StatusLineProps = {
  suspended: boolean;
  status: ReadyType;
};

const StatusLine = styled.div<StatusLineProps>

This lets you pass down additional custom props to your styled components without Typescript complaining about it. Hooray!

4. I tried to move all the relevant numbers into variables as high up as possible. My hope is that you (mostly) won't have to know d3 or svg stuff AT ALL in order to make small styling or sizing changes.

[joshri]

Hello one and all (@opudrovs @ozamosi @JamWils )

I'm bringing you into this draft PR because I wanted to show where the styling is at before spending the rest of today (and probs tomorrow) on the drag + zoom functionality. Also it's a LOT of changes so I wanted to get it out there early.

[joshri]

Zoom is back

Zoom.mov

[joshri]

Drag is back - the scaling isn't perfect (your mouse doesn't move 1:1 with the graph) but it's close enough imo.

Drag.mov

[JamWils]

Great work @joshri

[opudrovs]

Great job, @joshri ! The code is much simpler, cleaner, and easier to modify than with the Dagre library.

A couple of minor issues:

1. When the vertical scrollbar is not scrolled to top, there is flickering when zooming to 0% and back to a non-zero value. I was able to reproduce the issue consistently in Chrome.

graph_1.mov

Besides, at the 0 zoomPercent the graph suddenly disappears from screen.

I suggest mapping the zoomPercent value received from the slider and passed as a prop to the DirectedGraph from the range of [0, 100] to [1, 100] and using this mapped zoom percent for all subsequent calculations, smth. like that (actually, I've copied the first range mapping function I found):

function mapValueToRange(value, inMin, inMax, outMin, outMax) {
  return ((value - inMin) * (outMax - outMin)) / (inMax - inMin) + outMin;
}

  const sanitizedZoomPercent = mapValueToRange(zoomPercent, 0, 100, 1, 100);

  //minimum zoomBox is 1000
  const zoomBox = 15000 - 14000 * (sanitizedZoomPercent / 100);
  //since viewbox is so large, make smaller mouse movements correspond to larger pan
  const panScale = 300 / sanitizedZoomPercent;
  

If this/a similar solution works, then mapValueToRange should be added to lib/utils, probably.

2. When first dragging the graph to a side and then changing the zoom values with the slider, the graph looks like it is moving on a curve:

graph_2.mov

I haven't looked how to make the graph maintain its position while being zoomed after dragging and I'm not sure if it is important to fix right now, but movement "on a curve" should be related to this line: const panScale = 300 / zoomPercent;

[joshri]

1. Zoom has been set to a minimum of 5 via a prop on the Mui Slider!
2. To fix the zoom curving - unfortunately I need a ref to access the getBoundingClientRect method on the svg. This gives us an accurate ratio for the panning rather than the one I just made up (300/zoompercent). The good news is that it doesn't need a useEffect. Also I promise this <SVGSVGElement> type is correct. Typescript complained until I put it in. The first SVG is the SVG category, and then the second one is the actual svg. Another example would be <SVGGElement> or <SVGRectElement>.

const svgRef = React.useRef<SVGSVGElement>();
  let panScale = 1;
  if (svgRef.current) {
    panScale = zoomBox / svgRef.current.getBoundingClientRect().width;
  }

[opudrovs]

Zoom has been set to a minimum of 5 via a prop on the Mui Slider!

Cool, much simpler fix than what I suggested. 🎉

To fix the zoom curving - unfortunately I need a ref to access the getBoundingClientRect method on the svg.

Great fix, can confirm that the graph now maintains drag position on zoom.

[opudrovs]

The code looks fine to me. I only see a few minor issues remaining.

3. The main issue is that, IMHO, the new lines/edges are somewhat misleading comparing to the old lines.

Old:

New:

With the old lines you could clearly see that all top non-root nodes are connected to the root object. In the new version, they are connected to each other. Is it OK?

[opudrovs]

4. It looks like the borders of the node elements are more rounded in the new version. Is it by design?

Old:

New:

Besides, the green thing over the border looks a bit weird at large zoom:

[opudrovs]

5. If the text is long, the checkmark icons sometimes look a bit squished:

I wonder if we need to make the nodes wider and/or make more space for text?

Maybe it all is not important to fix it all right now and I am nitpicking already.

[joshri]

3. The intersection of the lines is SUPPOSED to be covered by the root node - small bug there I'll have to look into how the path is being drawn exactly. I think I have to add in the horizontal separation bc I added that in post-path.
4. You are right about the status line - I wasn't super focused on design this PR, just trying to get it close to what we have now, but I will decrease the border radius to get it closer
5. That's definitely a case we need to handle - I will fix the icon. But yes I'm expecting a node redesign soon so I wasn't super focused on it.

[opudrovs]

3. The intersection of the lines is SUPPOSED to be covered by the root node - small bug there I'll have to look into how the path is being drawn exactly. I think I have to add in the horizontal separation bc I added that in post-path.

The intersection of the lines is fine with me. I don't like that top level non-root objects look like they are connected to each other (because the lines that connect each of these objects to the root object overlap).

[opudrovs]

3. Under the root object I mean the Kustomization, not GitRepository. With the line from GitRepository to Kustomization everything is fine.

[opudrovs]

Great! I have no other questions except for: @JamWils , @ozamosi , what do you think of the new lines here:

#2476 (comment)

and the initial zoom:

If both are OK with you, I am approving the PR.

[opudrovs]

If we replace

return (
  <path
    key={index}
    d={`M${l.source.x} ${
      l.source.y + nodeSize.verticalSeparation
    }H${l.target.x}V${l.target.y + nodeSize.verticalSeparation}`}
  />
);

with

return l.source.x === l.target.x ? (
  <path
    key={index}
    d={`M${l.source.x} ${
      l.source.y + nodeSize.verticalSeparation
    }V${l.target.y + nodeSize.verticalSeparation}`}
  />
) : (
  <path
    key={index}
    d={`M${l.source.x} ${
      l.source.y + nodeSize.verticalSeparation
    }L${l.target.x} ${
      l.target.y - nodeSize.verticalSeparation / 2
    }V${l.target.y + nodeSize.verticalSeparation}`}
  />
);

(using only the second part without condition would work too, but I'm not sure if there could be any artifacts under some conditions)
we can make lines look like that:

The only issue left is that the nodes are not centered relative to the root node, but it is not related to making the lines diagonal.

[opudrovs]

@joshri , @ozamosi said that there was a ticket ( #2484 ) about changing the shape of the lines (what I had not seen).

In this case the only issue remaining is centering non-root nodes horizontally (if there is an even number of columns) relative to the root node.

[ozamosi]

Looking carefully at #2484, that ticket describes a path that goes straight down, straight to the side, straight down again, whereas this PR only out and then down. So it's not quite the "right" shape (but then this branch doesn't promise to implement that shape).

The only issue left is that the nodes are not centered relative to the root node

I think the issue here is that the boxes have irregular spacing. It looks like if the child node has children of its own (like helm-controller, kustomize-controller in @opudrovs's screenshot), then the gap before we draw the next child is 2 boxes' width, whereas if the child node has no children (like webhook-receiver) then the next node is drawn much closer. So in your screenshot, it's "centered" for an even number of boxes, but there's an empty box on the left side of the parent node.

Do I make any sense?

[opudrovs]

Great observations, @ozamosi !

[opudrovs]

As we agreed, lines and node positioning can be handled in a separate ticket.

LGTM in general, removing the Dagre library and switching to pure D3.js is a huge step forward for the graph! 🎉