From 4b9462993ee3ad47edf96cbdd6a2228720c6aedc Mon Sep 17 00:00:00 2001
From: Alexander Zobnin <alexanderzobnin@gmail.com>
Date: Mon, 22 Oct 2018 14:17:46 +0300
Subject: [PATCH] graph legend: refactor, fix another review issues

---
 .../colorpicker/SeriesColorPicker.tsx         | 102 ++++++++++--------
 .../colorpicker/SeriesColorPickerPopover.tsx  |  70 ++++++++++++
 .../colorpicker/withColorPicker.tsx           |  83 --------------
 public/app/core/core.ts                       |   2 +-
 .../app/plugins/panel/graph/Legend/Legend.tsx |  14 +--
 .../panel/graph/Legend/LegendSeriesItem.tsx   |  17 +--
 .../panel/graph/series_overrides_ctrl.ts      |   2 +-
 7 files changed, 142 insertions(+), 148 deletions(-)
 create mode 100644 public/app/core/components/colorpicker/SeriesColorPickerPopover.tsx
 delete mode 100644 public/app/core/components/colorpicker/withColorPicker.tsx

diff --git a/public/app/core/components/colorpicker/SeriesColorPicker.tsx b/public/app/core/components/colorpicker/SeriesColorPicker.tsx
index 9abd3574ae1..ae744da423d 100644
--- a/public/app/core/components/colorpicker/SeriesColorPicker.tsx
+++ b/public/app/core/components/colorpicker/SeriesColorPicker.tsx
@@ -1,67 +1,81 @@
 import React from 'react';
-import { ColorPickerPopover } from './ColorPickerPopover';
-import { react2AngularDirective } from 'app/core/utils/react2angular';
+import ReactDOM from 'react-dom';
+import Drop from 'tether-drop';
+import { SeriesColorPickerPopover } from './SeriesColorPickerPopover';
 
 export interface SeriesColorPickerProps {
-  // series: any;
   color: string;
   yaxis?: number;
-  onColorChange: (color: string) => void;
+  optionalClass?: string;
+  onColorChange: (newColor: string) => void;
   onToggleAxis?: () => void;
 }
 
-export class SeriesColorPicker extends React.PureComponent<SeriesColorPickerProps, any> {
-  render() {
-    return (
-      <div className="graph-legend-popover">
-        {this.props.yaxis && <AxisSelector yaxis={this.props.yaxis} onToggleAxis={this.props.onToggleAxis} />}
-        <ColorPickerPopover color={this.props.color} onColorSelect={this.props.onColorChange} />
-      </div>
-    );
-  }
-}
+export class SeriesColorPicker extends React.Component<SeriesColorPickerProps> {
+  pickerElem: any;
+  colorPickerDrop: any;
 
-interface AxisSelectorProps {
-  yaxis: number;
-  onToggleAxis: () => void;
-}
+  static defaultProps = {
+    optionalClass: '',
+    yaxis: undefined,
+    onToggleAxis: () => {},
+  };
 
-interface AxisSelectorState {
-  yaxis: number;
-}
-
-export class AxisSelector extends React.PureComponent<AxisSelectorProps, AxisSelectorState> {
   constructor(props) {
     super(props);
-    this.state = {
-      yaxis: this.props.yaxis,
-    };
-    this.onToggleAxis = this.onToggleAxis.bind(this);
+    this.openColorPicker = this.openColorPicker.bind(this);
   }
 
-  onToggleAxis() {
-    this.setState({
-      yaxis: this.state.yaxis === 2 ? 1 : 2,
+  openColorPicker() {
+    if (this.colorPickerDrop) {
+      this.destroyDrop();
+    }
+
+    const { color, yaxis, onColorChange, onToggleAxis } = this.props;
+    const dropContent = (
+      <SeriesColorPickerPopover color={color} yaxis={yaxis} onColorChange={onColorChange} onToggleAxis={onToggleAxis} />
+    );
+    const dropContentElem = document.createElement('div');
+    ReactDOM.render(dropContent, dropContentElem);
+
+    const drop = new Drop({
+      target: this.pickerElem,
+      content: dropContentElem,
+      position: 'top center',
+      classes: 'drop-popover',
+      openOn: 'hover',
+      hoverCloseDelay: 200,
+      remove: true,
+      tetherOptions: {
+        constraints: [{ to: 'scrollParent', attachment: 'none both' }],
+      },
     });
-    this.props.onToggleAxis();
+
+    drop.on('close', this.closeColorPicker.bind(this));
+
+    this.colorPickerDrop = drop;
+    this.colorPickerDrop.open();
+  }
+
+  closeColorPicker() {
+    setTimeout(() => {
+      this.destroyDrop();
+    }, 100);
+  }
+
+  destroyDrop() {
+    if (this.colorPickerDrop && this.colorPickerDrop.tether) {
+      this.colorPickerDrop.destroy();
+      this.colorPickerDrop = null;
+    }
   }
 
   render() {
-    const leftButtonClass = this.state.yaxis === 1 ? 'btn-success' : 'btn-inverse';
-    const rightButtonClass = this.state.yaxis === 2 ? 'btn-success' : 'btn-inverse';
-
+    const { optionalClass, children } = this.props;
     return (
-      <div className="p-b-1">
-        <label className="small p-r-1">Y Axis:</label>
-        <button onClick={this.onToggleAxis} className={'btn btn-small ' + leftButtonClass}>
-          Left
-        </button>
-        <button onClick={this.onToggleAxis} className={'btn btn-small ' + rightButtonClass}>
-          Right
-        </button>
+      <div className={optionalClass} ref={e => (this.pickerElem = e)} onClick={this.openColorPicker}>
+        {children}
       </div>
     );
   }
 }
-
-react2AngularDirective('seriesColorPicker', SeriesColorPicker, ['series', 'onColorChange', 'onToggleAxis']);
diff --git a/public/app/core/components/colorpicker/SeriesColorPickerPopover.tsx b/public/app/core/components/colorpicker/SeriesColorPickerPopover.tsx
new file mode 100644
index 00000000000..085d554300d
--- /dev/null
+++ b/public/app/core/components/colorpicker/SeriesColorPickerPopover.tsx
@@ -0,0 +1,70 @@
+import React from 'react';
+import { ColorPickerPopover } from './ColorPickerPopover';
+import { react2AngularDirective } from 'app/core/utils/react2angular';
+
+export interface SeriesColorPickerPopoverProps {
+  color: string;
+  yaxis?: number;
+  onColorChange: (color: string) => void;
+  onToggleAxis?: () => void;
+}
+
+export class SeriesColorPickerPopover extends React.PureComponent<SeriesColorPickerPopoverProps, any> {
+  render() {
+    return (
+      <div className="graph-legend-popover">
+        {this.props.yaxis && <AxisSelector yaxis={this.props.yaxis} onToggleAxis={this.props.onToggleAxis} />}
+        <ColorPickerPopover color={this.props.color} onColorSelect={this.props.onColorChange} />
+      </div>
+    );
+  }
+}
+
+interface AxisSelectorProps {
+  yaxis: number;
+  onToggleAxis: () => void;
+}
+
+interface AxisSelectorState {
+  yaxis: number;
+}
+
+export class AxisSelector extends React.PureComponent<AxisSelectorProps, AxisSelectorState> {
+  constructor(props) {
+    super(props);
+    this.state = {
+      yaxis: this.props.yaxis,
+    };
+    this.onToggleAxis = this.onToggleAxis.bind(this);
+  }
+
+  onToggleAxis() {
+    this.setState({
+      yaxis: this.state.yaxis === 2 ? 1 : 2,
+    });
+    this.props.onToggleAxis();
+  }
+
+  render() {
+    const leftButtonClass = this.state.yaxis === 1 ? 'btn-success' : 'btn-inverse';
+    const rightButtonClass = this.state.yaxis === 2 ? 'btn-success' : 'btn-inverse';
+
+    return (
+      <div className="p-b-1">
+        <label className="small p-r-1">Y Axis:</label>
+        <button onClick={this.onToggleAxis} className={'btn btn-small ' + leftButtonClass}>
+          Left
+        </button>
+        <button onClick={this.onToggleAxis} className={'btn btn-small ' + rightButtonClass}>
+          Right
+        </button>
+      </div>
+    );
+  }
+}
+
+react2AngularDirective('seriesColorPickerPopover', SeriesColorPickerPopover, [
+  'series',
+  'onColorChange',
+  'onToggleAxis',
+]);
diff --git a/public/app/core/components/colorpicker/withColorPicker.tsx b/public/app/core/components/colorpicker/withColorPicker.tsx
deleted file mode 100644
index d0567fe4e18..00000000000
--- a/public/app/core/components/colorpicker/withColorPicker.tsx
+++ /dev/null
@@ -1,83 +0,0 @@
-import React from 'react';
-import ReactDOM from 'react-dom';
-import Drop from 'tether-drop';
-import { SeriesColorPicker } from './SeriesColorPicker';
-
-export interface WithSeriesColorPickerProps {
-  color: string;
-  yaxis?: number;
-  optionalClass?: string;
-  onColorChange: (newColor: string) => void;
-  onToggleAxis?: () => void;
-}
-
-export default function withSeriesColorPicker(WrappedComponent) {
-  return class extends React.Component<WithSeriesColorPickerProps, any> {
-    pickerElem: any;
-    colorPickerDrop: any;
-
-    static defaultProps = {
-      optionalClass: '',
-      yaxis: undefined,
-      onToggleAxis: () => {},
-    };
-
-    constructor(props) {
-      super(props);
-      this.openColorPicker = this.openColorPicker.bind(this);
-    }
-
-    openColorPicker() {
-      if (this.colorPickerDrop) {
-        this.destroyDrop();
-      }
-
-      const { color, yaxis, onColorChange, onToggleAxis } = this.props;
-      const dropContent = (
-        <SeriesColorPicker color={color} yaxis={yaxis} onColorChange={onColorChange} onToggleAxis={onToggleAxis} />
-      );
-      const dropContentElem = document.createElement('div');
-      ReactDOM.render(dropContent, dropContentElem);
-
-      const drop = new Drop({
-        target: this.pickerElem,
-        content: dropContentElem,
-        position: 'top center',
-        classes: 'drop-popover',
-        openOn: 'hover',
-        hoverCloseDelay: 200,
-        remove: true,
-        tetherOptions: {
-          constraints: [{ to: 'scrollParent', attachment: 'none both' }],
-        },
-      });
-
-      drop.on('close', this.closeColorPicker.bind(this));
-
-      this.colorPickerDrop = drop;
-      this.colorPickerDrop.open();
-    }
-
-    closeColorPicker() {
-      setTimeout(() => {
-        this.destroyDrop();
-      }, 100);
-    }
-
-    destroyDrop() {
-      if (this.colorPickerDrop && this.colorPickerDrop.tether) {
-        this.colorPickerDrop.destroy();
-        this.colorPickerDrop = null;
-      }
-    }
-
-    render() {
-      const { optionalClass, onColorChange, ...wrappedComponentProps } = this.props;
-      return (
-        <div className={optionalClass} ref={e => (this.pickerElem = e)} onClick={this.openColorPicker}>
-          <WrappedComponent {...wrappedComponentProps} />
-        </div>
-      );
-    }
-  };
-}
diff --git a/public/app/core/core.ts b/public/app/core/core.ts
index 173d6b80b15..9a398a5ae5a 100644
--- a/public/app/core/core.ts
+++ b/public/app/core/core.ts
@@ -14,7 +14,7 @@ import './components/jsontree/jsontree';
 import './components/code_editor/code_editor';
 import './utils/outline';
 import './components/colorpicker/ColorPicker';
-import './components/colorpicker/SeriesColorPicker';
+import './components/colorpicker/SeriesColorPickerPopover';
 import './components/colorpicker/spectrum_picker';
 import './services/search_srv';
 import './services/ng_react';
diff --git a/public/app/plugins/panel/graph/Legend/Legend.tsx b/public/app/plugins/panel/graph/Legend/Legend.tsx
index ce770163a83..46d0b2a5906 100644
--- a/public/app/plugins/panel/graph/Legend/Legend.tsx
+++ b/public/app/plugins/panel/graph/Legend/Legend.tsx
@@ -87,16 +87,6 @@ export class GraphLegend extends React.PureComponent<GraphLegendProps, LegendSta
     };
   }
 
-  onToggleAxis = series => {
-    this.props.onToggleAxis(series);
-    this.forceUpdate();
-  };
-
-  onColorChange = (series, color) => {
-    this.props.onColorChange(series, color);
-    this.forceUpdate();
-  };
-
   sortLegend() {
     let seriesList = [...this.props.seriesList] || [];
     if (this.props.sort) {
@@ -187,9 +177,9 @@ export class GraphLegend extends React.PureComponent<GraphLegendProps, LegendSta
       seriesList: seriesList,
       hiddenSeries: hiddenSeries,
       onToggleSeries: this.onToggleSeries,
-      onToggleAxis: this.onToggleAxis,
+      onToggleAxis: this.props.onToggleAxis,
       onToggleSort: this.props.onToggleSort,
-      onColorChange: this.onColorChange,
+      onColorChange: this.props.onColorChange,
       ...seriesValuesProps,
       ...sortProps,
     };
diff --git a/public/app/plugins/panel/graph/Legend/LegendSeriesItem.tsx b/public/app/plugins/panel/graph/Legend/LegendSeriesItem.tsx
index 979fde6f828..6c50e4f1c58 100644
--- a/public/app/plugins/panel/graph/Legend/LegendSeriesItem.tsx
+++ b/public/app/plugins/panel/graph/Legend/LegendSeriesItem.tsx
@@ -1,6 +1,6 @@
 import React from 'react';
 import { TimeSeries } from 'app/core/core';
-import withColorPicker from 'app/core/components/colorpicker/withColorPicker';
+import { SeriesColorPicker } from 'app/core/components/colorpicker/SeriesColorPicker';
 
 export const LEGEND_STATS = ['min', 'max', 'avg', 'current', 'total'];
 
@@ -55,6 +55,10 @@ export class LegendItem extends React.PureComponent<LegendItemProps, LegendItemS
 
   onColorChange = color => {
     this.props.onColorChange(this.props.series, color);
+    // Because of PureComponent nature it makes only shallow props comparison and changing of series.color doesn't run
+    // component re-render. In this case we can't rely on color, selected by user, because it may be overwritten
+    // by series overrides. So we need to use forceUpdate() to make sure we have proper series color.
+    this.forceUpdate();
   };
 
   render() {
@@ -156,17 +160,16 @@ class LegendSeriesIcon extends React.PureComponent<LegendSeriesIconProps, Legend
   };
 
   render() {
-    const { yaxis } = this.props;
-    const IconWithColorPicker = withColorPicker(SeriesIcon);
-
     return (
-      <IconWithColorPicker
+      <SeriesColorPicker
         optionalClass="graph-legend-icon"
-        yaxis={yaxis}
+        yaxis={this.props.yaxis}
         color={this.state.color}
         onColorChange={this.onColorChange}
         onToggleAxis={this.props.onToggleAxis}
-      />
+      >
+        <SeriesIcon color={this.props.color} />
+      </SeriesColorPicker>
     );
   }
 }
diff --git a/public/app/plugins/panel/graph/series_overrides_ctrl.ts b/public/app/plugins/panel/graph/series_overrides_ctrl.ts
index deb7bd8ba61..33520cb403b 100644
--- a/public/app/plugins/panel/graph/series_overrides_ctrl.ts
+++ b/public/app/plugins/panel/graph/series_overrides_ctrl.ts
@@ -53,7 +53,7 @@ export function SeriesOverridesCtrl($scope, $element, popoverSrv) {
       element: $element.find('.dropdown')[0],
       position: 'top center',
       openOn: 'click',
-      template: '<series-color-picker series="series" onColorChange="colorSelected" />',
+      template: '<series-color-picker-popover series="series" onColorChange="colorSelected" />',
       model: {
         autoClose: true,
         colorSelected: $scope.colorSelected,