From 68df3e561c41bc0fc6c205a188439d0cd0e2a261 Mon Sep 17 00:00:00 2001 From: Bill Neff Date: Sun, 9 Oct 2016 22:39:19 -0400 Subject: [PATCH] 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 | 58 +++++++++++++++++++-------- src/chart/generateCategoricalChart.js | 5 ++- test/specs/chart/LineChartSpec.js | 26 ++++++++++++ 3 files changed, 72 insertions(+), 17 deletions(-) diff --git a/src/cartesian/Brush.js b/src/cartesian/Brush.js index d8828f3aec..e919b6557e 100644 --- a/src/cartesian/Brush.js +++ b/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 = { diff --git a/src/chart/generateCategoricalChart.js b/src/chart/generateCategoricalChart.js index a64de66dc3..30f2842818 100644 --- a/src/chart/generateCategoricalChart.js +++ b/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, diff --git a/test/specs/chart/LineChartSpec.js b/test/specs/chart/LineChartSpec.js index 49c9017805..3870af40b2 100644 --- a/test/specs/chart/LineChartSpec.js +++ b/test/specs/chart/LineChartSpec.js @@ -408,6 +408,32 @@ describe(' - 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 }]); + + }); + });