Review feedback

client/app/scripts/charts/__tests__/node-layout-test.js

@@ -6,18 +6,21 @@ import { fromJS, Map } from 'immutable';
 describe('NodesLayout', () => {
   const NodesLayout = require('../nodes-layout');
 
-  function scale(val) {
-    return val * 3;
+  function getNodeCoordinates(nodes) {
+    const coords = [];
+    nodes
+      .sortBy(node => node.get('id'))
+      .forEach(node => {
+        coords.push(node.get('x'));
+        coords.push(node.get('y'));
+      });
+    return coords;
   }
-  const topologyId = 'tid';
-  const width = 80;
-  const height = 80;
-  const margins = {
-    left: 0,
-    top: 0
-  };
+
   let options;
   let nodes;
+  let coords;
+  let resultCoords;
 
   const nodeSets = {
     initial4: {
@@ -122,6 +125,7 @@ describe('NodesLayout', () => {
     options.cachedLayout = result;
     options.nodeCache = options.nodeCache.merge(result.nodes);
     options.edgeCache = options.edgeCache.merge(result.edge);
+    coords = getNodeCoordinates(result.nodes);
 
     result = NodesLayout.doLayout(
       nodeSets.removeEdge24.nodes,
@@ -131,12 +135,8 @@ describe('NodesLayout', () => {
     nodes = result.nodes.toJS();
     // console.log('remove 1 edge', nodes, result);
 
-    expect(nodes.n1.x).toBeLessThan(nodes.n2.x);
-    expect(nodes.n1.y).toEqual(nodes.n2.y);
-    expect(nodes.n1.x).toEqual(nodes.n3.x);
-    expect(nodes.n1.y).toBeLessThan(nodes.n3.y);
-    expect(nodes.n3.x).toBeLessThan(nodes.n4.x);
-    expect(nodes.n3.y).toEqual(nodes.n4.y);
+    resultCoords = getNodeCoordinates(result.nodes);
+    expect(resultCoords).toEqual(coords);
   });
 
   it('keeps nodes in rectangle after removed edge reappears', () => {
@@ -144,6 +144,7 @@ describe('NodesLayout', () => {
       nodeSets.initial4.nodes,
       nodeSets.initial4.edges);
 
+    coords = getNodeCoordinates(result.nodes);
     options.cachedLayout = result;
     options.nodeCache = options.nodeCache.merge(result.nodes);
     options.edgeCache = options.edgeCache.merge(result.edge);
@@ -165,12 +166,8 @@ describe('NodesLayout', () => {
     nodes = result.nodes.toJS();
     // console.log('re-add 1 edge', nodes, result);
 
-    expect(nodes.n1.x).toBeLessThan(nodes.n2.x);
-    expect(nodes.n1.y).toEqual(nodes.n2.y);
-    expect(nodes.n1.x).toEqual(nodes.n3.x);
-    expect(nodes.n1.y).toBeLessThan(nodes.n3.y);
-    expect(nodes.n3.x).toBeLessThan(nodes.n4.x);
-    expect(nodes.n3.y).toEqual(nodes.n4.y);
+    resultCoords = getNodeCoordinates(result.nodes);
+    expect(resultCoords).toEqual(coords);
   });
 
   it('keeps nodes in rectangle after node dissappears', () => {
@@ -189,10 +186,9 @@ describe('NodesLayout', () => {
 
     nodes = result.nodes.toJS();
 
-    expect(nodes.n1.x).toEqual(nodes.n3.x);
-    expect(nodes.n1.y).toBeLessThan(nodes.n3.y);
-    expect(nodes.n3.x).toBeLessThan(nodes.n4.x);
-    expect(nodes.n3.y).toEqual(nodes.n4.y);
+    resultCoords = getNodeCoordinates(result.nodes);
+    expect(resultCoords.slice(0,2)).toEqual(coords.slice(0,2));
+    expect(resultCoords.slice(2,6)).toEqual(coords.slice(4,8));
   });
 
   it('keeps nodes in rectangle after removed node reappears', () => {
@@ -202,6 +198,7 @@ describe('NodesLayout', () => {
 
     nodes = result.nodes.toJS();
 
+    coords = getNodeCoordinates(result.nodes);
     options.cachedLayout = result;
     options.nodeCache = options.nodeCache.merge(result.nodes);
     options.edgeCache = options.edgeCache.merge(result.edge);
@@ -229,10 +226,9 @@ describe('NodesLayout', () => {
     nodes = result.nodes.toJS();
     // console.log('re-add 1 node', nodes);
 
-    expect(nodes.n1.x).toEqual(nodes.n3.x);
-    expect(nodes.n1.y).toBeLessThan(nodes.n3.y);
-    expect(nodes.n3.x).toBeLessThan(nodes.n4.x);
-    expect(nodes.n3.y).toEqual(nodes.n4.y);
+    resultCoords = getNodeCoordinates(result.nodes);
+    expect(resultCoords.slice(0,2)).toEqual(coords.slice(0,2));
+    expect(resultCoords.slice(2,6)).toEqual(coords.slice(4,8));
   });
 
 });

client/app/scripts/charts/nodes-chart.js

@@ -2,7 +2,6 @@ const _ = require('lodash');
 const d3 = require('d3');
 const debug = require('debug')('scope:nodes-chart');
 const React = require('react');
-const makeList = require('immutable').List;
 const makeMap = require('immutable').Map;
 const timely = require('timely');
 const Spring = require('react-motion').Spring;
@@ -32,7 +31,6 @@ const NodesChart = React.createClass({
     return {
       nodes: makeMap(),
       edges: makeMap(),
-      history: makeList(),
       nodeScale: d3.scale.linear(),
       shiftTranslate: [0, 0],
       panTranslate: [0, 0],
@@ -177,9 +175,10 @@ const NodesChart = React.createClass({
   },
 
   renderMaxNodesError: function(show) {
+    const errorHint = 'We\u0027re working on it, but for now, try a different view?';
     return (
       <NodesError faIconClass="fa-ban" hidden={!show}>
-        <div className="centered">Too many nodes to show in the browser.<br />We're working on it, but for now, try a different view?</div>
+        <div className="centered">Too many nodes to show in the browser.<br />{errorHint}</div>
       </NodesError>
     );
   },
@@ -455,8 +454,7 @@ const NodesChart = React.createClass({
       height: props.height,
       scale: nodeScale,
       margins: MARGINS,
-      topologyId: this.props.topologyId,
-      history: state.history
+      topologyId: this.props.topologyId
     };
 
     const timedLayouter = timely(NodesLayout.doLayout);

client/app/scripts/charts/nodes-layout.js

@@ -246,9 +246,11 @@ export function doLayout(nodes, edges, opts) {
   }
 
   // cache results
-  cache.cachedLayout = layout;
-  cache.nodeCache = cache.nodeCache.merge(layout.nodes);
-  cache.edgeCache = cache.edgeCache.merge(layout.edges);
+  if (layout) {
+    cache.cachedLayout = layout;
+    cache.nodeCache = cache.nodeCache.merge(layout.nodes);
+    cache.edgeCache = cache.edgeCache.merge(layout.edges);
+  }
 
   return layout;
 }