From fc90ec8929cb1d99d6f30a5933e9493134a937f5 Mon Sep 17 00:00:00 2001
From: Roland Schilter <roli@weave.works>
Date: Fri, 28 Jul 2017 11:52:30 +0200
Subject: [PATCH] JS feedback from David

---
 client/app/scripts/charts/nodes-grid.js           |  4 ----
 client/app/scripts/components/node-details.js     |  1 -
 .../node-details/node-details-health-link-item.js | 11 +++--------
 .../node-details/node-details-health.js           |  2 --
 .../node-details/node-details-table-node-link.js  |  5 ++++-
 .../node-details-table-node-metric-link.js        | 15 +++------------
 .../node-details/node-details-table-row.js        | 13 +++++++++++++
 .../components/node-details/node-details-table.js |  5 ++++-
 client/app/styles/_base.scss                      |  2 +-
 client/package.json                               |  2 +-
 client/yarn.lock                                  |  2 +-
 11 files changed, 30 insertions(+), 32 deletions(-)

diff --git a/client/app/scripts/charts/nodes-grid.js b/client/app/scripts/charts/nodes-grid.js
index 9c6793951..dee2b08bd 100644
--- a/client/app/scripts/charts/nodes-grid.js
+++ b/client/app/scripts/charts/nodes-grid.js
@@ -88,10 +88,6 @@ class NodesGrid extends React.Component {
   }
 
   onClickRow(ev, node) {
-    // TODO: do this better
-    if (ev.target.className === 'node-details-table-node-link') {
-      return;
-    }
     trackMixpanelEvent('scope.node.click', {
       layout: TABLE_VIEW_MODE,
       topologyId: this.props.currentTopology.get('id'),
diff --git a/client/app/scripts/components/node-details.js b/client/app/scripts/components/node-details.js
index fc3368dba..d76ff800a 100644
--- a/client/app/scripts/components/node-details.js
+++ b/client/app/scripts/components/node-details.js
@@ -195,7 +195,6 @@ class NodeDetails extends React.Component {
             <NodeDetailsHealth
               metrics={details.metrics}
               topologyId={topologyId}
-              nodeColor={nodeColor}
               />
           </div>}
           {details.metadata && <div className="node-details-content-section">
diff --git a/client/app/scripts/components/node-details/node-details-health-link-item.js b/client/app/scripts/components/node-details/node-details-health-link-item.js
index 248417564..d6a43f662 100644
--- a/client/app/scripts/components/node-details/node-details-health-link-item.js
+++ b/client/app/scripts/components/node-details/node-details-health-link-item.js
@@ -4,7 +4,6 @@ import NodeDetailsHealthItem from './node-details-health-item';
 import CloudLink from '../cloud-link';
 import { getMetricColor } from '../../utils/metric-utils';
 import { trackMixpanelEvent } from '../../utils/tracking-utils';
-import { GRAPH_VIEW_MODE } from '../../constants/naming';
 
 export default class NodeDetailsHealthLinkItem extends React.Component {
 
@@ -28,15 +27,11 @@ export default class NodeDetailsHealthLinkItem extends React.Component {
   }
 
   onClick() {
-    trackMixpanelEvent('scope.node.metric.click', {
-      layout: GRAPH_VIEW_MODE,
-      topologyId: this.props.topologyId,
-    });
+    trackMixpanelEvent('scope.node.metric.click', { topologyId: this.props.topologyId });
   }
 
   render() {
-    const { id, nodeColor, url, ...props } = this.props;
-
+    const { id, url, ...props } = this.props;
     const metricColor = getMetricColor(id);
 
     return (
@@ -52,7 +47,7 @@ export default class NodeDetailsHealthLinkItem extends React.Component {
           {...props}
           hovered={this.state.hovered}
           metricColor={metricColor}
-          labelColor={this.state.hovered && nodeColor} />
+        />
       </CloudLink>
     );
   }
diff --git a/client/app/scripts/components/node-details/node-details-health.js b/client/app/scripts/components/node-details/node-details-health.js
index 5ceddca6b..102c92d62 100644
--- a/client/app/scripts/components/node-details/node-details-health.js
+++ b/client/app/scripts/components/node-details/node-details-health.js
@@ -24,7 +24,6 @@ export default class NodeDetailsHealth extends React.Component {
     const {
       metrics = makeList(),
       topologyId,
-      nodeColor,
     } = this.props;
 
     const primeCutoff = metrics.length > 3 && !this.state.expanded ? 2 : metrics.length;
@@ -42,7 +41,6 @@ export default class NodeDetailsHealth extends React.Component {
             {...item}
             key={item.id}
             topologyId={topologyId}
-            nodeColor={nodeColor}
           />)}
           {showOverflow && <NodeDetailsHealthOverflow
             items={overflowMetrics}
diff --git a/client/app/scripts/components/node-details/node-details-table-node-link.js b/client/app/scripts/components/node-details/node-details-table-node-link.js
index b5c8404dc..bd77b9f3a 100644
--- a/client/app/scripts/components/node-details/node-details-table-node-link.js
+++ b/client/app/scripts/components/node-details/node-details-table-node-link.js
@@ -3,6 +3,7 @@ import { connect } from 'react-redux';
 
 import { clickRelative } from '../../actions/app-actions';
 import { trackMixpanelEvent } from '../../utils/tracking-utils';
+import { dismissRowClickProps } from './node-details-table-row';
 
 
 class NodeDetailsTableNodeLink extends React.Component {
@@ -38,7 +39,9 @@ class NodeDetailsTableNodeLink extends React.Component {
       return (
         <span
           className="node-details-table-node-link" title={title}
-          ref={this.saveNodeRef} onClick={this.handleClick}>
+          ref={this.saveNodeRef} onClick={this.handleClick}
+          {...dismissRowClickProps}
+        >
           {label}
         </span>
       );
diff --git a/client/app/scripts/components/node-details/node-details-table-node-metric-link.js b/client/app/scripts/components/node-details/node-details-table-node-metric-link.js
index 51d9ee455..db5090604 100644
--- a/client/app/scripts/components/node-details/node-details-table-node-metric-link.js
+++ b/client/app/scripts/components/node-details/node-details-table-node-metric-link.js
@@ -3,7 +3,7 @@ import React from 'react';
 import CloudLink from '../cloud-link';
 import { formatMetric } from '../../utils/string-utils';
 import { trackMixpanelEvent } from '../../utils/tracking-utils';
-import { TABLE_VIEW_MODE } from '../../constants/naming';
+import { dismissRowClickProps } from './node-details-table-row';
 
 class NodeDetailsTableNodeMetricLink extends React.Component {
   constructor(props) {
@@ -13,15 +13,7 @@ class NodeDetailsTableNodeMetricLink extends React.Component {
   }
 
   onClick() {
-    trackMixpanelEvent('scope.node.metric.click', {
-      layout: TABLE_VIEW_MODE,
-      topologyId: this.props.topologyId,
-    });
-  }
-
-  static dismissEvent(ev) {
-    ev.preventDefault();
-    ev.stopPropagation();
+    trackMixpanelEvent('scope.node.metric.click', { topologyId: this.props.topologyId });
   }
 
   render() {
@@ -31,8 +23,7 @@ class NodeDetailsTableNodeMetricLink extends React.Component {
       <td
         className="node-details-table-node-metric"
         style={style}
-        // Skip onMouseUp event for table row
-        onMouseUp={NodeDetailsTableNodeMetricLink.dismissEvent}
+        {...dismissRowClickProps}
       >
         <CloudLink
           alwaysShow
diff --git a/client/app/scripts/components/node-details/node-details-table-row.js b/client/app/scripts/components/node-details/node-details-table-row.js
index 4b910d943..cd9fca2a0 100644
--- a/client/app/scripts/components/node-details/node-details-table-row.js
+++ b/client/app/scripts/components/node-details/node-details-table-row.js
@@ -89,6 +89,19 @@ function renderValues(node, columns = [], columnStyles = [], timestamp = null, t
   });
 }
 
+/**
+ * Table row children may react to onClick events but the row
+ * itself does detect a click by looking at onMouseUp. To stop
+ * the bubbling of clicks on child elements we need to dismiss
+ * the onMouseUp event.
+ */
+export const dismissRowClickProps = {
+  onMouseUp: (ev) => {
+    ev.preventDefault();
+    ev.stopPropagation();
+  }
+};
+
 export default class NodeDetailsTableRow extends React.Component {
   constructor(props, context) {
     super(props, context);
diff --git a/client/app/scripts/components/node-details/node-details-table.js b/client/app/scripts/components/node-details/node-details-table.js
index d5f41f270..928718385 100644
--- a/client/app/scripts/components/node-details/node-details-table.js
+++ b/client/app/scripts/components/node-details/node-details-table.js
@@ -108,8 +108,11 @@ function sortNodes(nodes, getValue, sortedDesc) {
 function getSortedNodes(nodes, sortedByHeader, sortedDesc) {
   const getValue = getValueForSortedBy(sortedByHeader);
   const withAndWithoutValues = groupBy(nodes, (n) => {
+    if (!n || n.valueEmpty) {
+      return 'withoutValues';
+    }
     const v = getValue(n);
-    return !n.valueEmpty && v !== null && v !== undefined ? 'withValues' : 'withoutValues';
+    return v !== null && v !== undefined ? 'withValues' : 'withoutValues';
   });
   const withValues = sortNodes(withAndWithoutValues.withValues, getValue, sortedDesc);
   const withoutValues = sortNodes(withAndWithoutValues.withoutValues, getValue, sortedDesc);
diff --git a/client/app/styles/_base.scss b/client/app/styles/_base.scss
index 860989829..33c3ba19b 100644
--- a/client/app/styles/_base.scss
+++ b/client/app/styles/_base.scss
@@ -956,7 +956,7 @@ a {
     }
 
     &-link-item {
-      @extend .palable;
+      @extend .btn-opacity;
       cursor: pointer;
       opacity: $link-opacity-default;
       width: 33%;
diff --git a/client/package.json b/client/package.json
index 3245191ef..c1f96fa8b 100644
--- a/client/package.json
+++ b/client/package.json
@@ -23,7 +23,7 @@
     "dagre": "0.7.4",
     "debug": "2.6.6",
     "filesize": "3.5.9",
-    "filter-invalid-dom-props": "^2.0.0",
+    "filter-invalid-dom-props": "2.0.0",
     "font-awesome": "4.7.0",
     "immutable": "3.8.1",
     "lcp": "1.1.0",
diff --git a/client/yarn.lock b/client/yarn.lock
index a4cc86908..d57cd128a 100644
--- a/client/yarn.lock
+++ b/client/yarn.lock
@@ -2499,7 +2499,7 @@ fill-range@^2.1.0:
     repeat-element "^1.1.2"
     repeat-string "^1.5.2"
 
-filter-invalid-dom-props@^2.0.0:
+filter-invalid-dom-props@2.0.0:
   version "2.0.0"
   resolved "https://registry.yarnpkg.com/filter-invalid-dom-props/-/filter-invalid-dom-props-2.0.0.tgz#527f1494cb3c4f282a73c43804153eb80c42dc2c"
   dependencies: