Modify Brush so that it can take a function or an array of functions …

…for onChange prop so that generateCategoricalChart does not clobber any user-supplied onChange prop

src/cartesian/Brush.js

@@ -4,12 +4,11 @@
 import React, { Component, PropTypes } from 'react';
 import classNames from 'classnames';
 import { scalePoint } from 'd3-scale';
-import pureRender from '../util/PureRender';
+import { shallowEqual } from '../util/PureRender';
 import Layer from '../container/Layer';
 import Text from '../component/Text';
 import _ from 'lodash';
 
-@pureRender
 class Brush extends Component {
 
   static displayName = 'Brush';
@@ -31,7 +30,7 @@ class Brush extends Component {
     tickFormatter: PropTypes.func,
     affectedCharts: PropTypes.oneOf(['all', 'self', 'others']),
     overlayChart: PropTypes.bool,
-    onChange: PropTypes.func,
+    onChange: PropTypes.oneOfType([PropTypes.func, PropTypes.array]),
   };
 
   static defaultProps = {
@@ -69,6 +68,13 @@ class Brush extends Component {
   componentWillReceiveProps(nextProps) {
     const { data, width, x, travellerWidth, startIndex, endIndex } = this.props;
 
+		// when affectedCharts==='others', the props might not change for start/endIndex
+		// even though the underlying value does.  Only update the state values
+		// if the props have changed from their prior values
+    if (startIndex !== nextProps.startIndex || endIndex !== nextProps.endIndex) {
+      this.setState({ startIndex, endIndex });
+    }
+
     if (nextProps.data !== data) {
       this.updateScale(nextProps);
       this.setState({ startIndex, endIndex });
@@ -84,6 +90,14 @@ class Brush extends Component {
     }
   }
 
+  shouldComponentUpdate({ onChange, ...restNextProps }, nextState) {
+		// onChange could be an array which might be created new each time, so deep equal check it
+    const { onChange: onChangeOld, ...restOldProps } = this.props;
+
+    return !shallowEqual(onChange, onChangeOld) ||
+			!shallowEqual(restNextProps, restOldProps) || !shallowEqual(nextState, this.state);
+  }
+
   componentWillUnmount() {
     this.scale = null;
     this.scaleValues = null;
@@ -124,6 +138,17 @@ class Brush extends Component {
     };
   }
 
+	/*
+	 * Get the startIndex and endIndex from props, unless
+	 * affectedCharts is others, in which case you need get it off of state
+	 */
+  getCurrentIndex() {
+    if (this.props.affectedCharts === 'others') {
+      return { startIndex: this.state.startIndex, endIndex: this.state.endIndex };
+    }
+    return { startIndex: this.props.startIndex, endIndex: this.props.endIndex };
+  }
+
   getTextOfTick(index) {
     const { data, tickFormatter, dataKey } = this.props;
     const text = (data[index] && dataKey) ? data[index][dataKey] : index;
@@ -196,15 +221,18 @@ class Brush extends Component {
       endX: endX + delta,
     });
 
+    const isIndexChange = !shallowEqual(newIndex, this.getCurrentIndex());
+
     this.setState({
       startX: startX + delta,
       endX: endX + delta,
       slideMoveStartX: e.pageX,
       ...newIndex,
     }, () => {
-      if (onChange) {
-        onChange(newIndex);
-      }
+      if (!isIndexChange) return;
+      if (_.isArray(onChange)) {
+        onChange.forEach((f) => f(newIndex));
+      } else { onChange(newIndex); }
     });
   }
 
@@ -234,14 +262,17 @@ class Brush extends Component {
     params[movingTravellerId] = prevValue + delta;
     const newIndex = this.getIndex(params);
 
+    const isIndexChange = !shallowEqual(newIndex, this.getCurrentIndex());
+
     this.setState({
       [movingTravellerId]: prevValue + delta,
       brushMoveStartX: e.pageX,
       ...newIndex,
     }, () => {
-      if (onChange) {
-        onChange(newIndex);
-      }
+      if (!isIndexChange) return;
+      if (_.isArray(onChange)) {
+        onChange.forEach((f) => f(newIndex));
+      } else { onChange(newIndex); }
     });
   }
 
@@ -341,15 +372,10 @@ class Brush extends Component {
   }
 
   renderText() {
-    const { y, height, travellerWidth, stroke, affectedCharts } = this.props;
-    let { startIndex, endIndex } = this.props;
+    const { y, height, travellerWidth, stroke } = this.props;
     const { startX, endX } = this.state;
 
-		// if only other charts are affected, need to use our own state for the text
-		// as we won't necessarily get new props from the chart
-    if (affectedCharts === 'others') {
-      ({ startIndex, endIndex } = this.state);
-    }
+    const { startIndex, endIndex } = this.getCurrentIndex();
 
     const offset = 5;
     const style = {

src/chart/generateCategoricalChart.js

@@ -861,8 +861,11 @@ const generateCategoricalChart = (ChartComponent, GraphicalChild) => {
         overrideHeight = { height: offset.height };
       }
 
+      const onChange = [this.handleBrushChangeForThis];
+      if (brushItem.props.onChange) onChange.push(brushItem.props.onChange);
+
       return React.cloneElement(brushItem, {
-        onChange: this.handleBrushChangeForThis,
+        onChange,
         data,
         x: offset.left,
         y,

test/specs/chart/LineChartSpec.js

@@ -408,6 +408,32 @@ describe('<LineChart /> - <Brush /> affectedCharts prop', () => {
 
   });
 
+  it('should call a supplied onChange function to Brush in addition to handleBrushChange', () => {
+
+    const onChangeSpy = sinon.spy();
+    const wrapper = mount(totalChart({ onChange: onChangeSpy }));
+
+    expect(onChangeSpy.callCount).to.equal(0);
+		// since props don't trickle down to Brush when affectedCharts="others"
+		// we can't cheat by using the Charts handleBrushChange method
+		// need to actually simulate mouse actions
+    const brush = wrapper.find(Brush).get(0);
+    brush.handleTravellerDown('startX', { pageX: brush.scale(0) });
+    brush.handleTravellerMove({ pageX: brush.scale(2) });
+    brush.handleUp();
+
+    expect(onChangeSpy.callCount).to.equal(1);
+    expect(onChangeSpy.getCall(0).args).to.eql([{ startIndex: 2, endIndex: 5 }]);
+
+    brush.handleTravellerDown('endX', { pageX: brush.scale(5) });
+    brush.handleTravellerMove({ pageX: brush.scale(4) });
+    brush.handleUp();
+
+    expect(onChangeSpy.callCount).to.equal(2);
+    expect(onChangeSpy.getCall(1).args).to.eql([{ startIndex: 2, endIndex: 4 }]);
+
+  });
+
 });