From 2d4098ccd8b8ca3fa8500d6276f0818a7bf5169b Mon Sep 17 00:00:00 2001 From: Marco Vettorello Date: Mon, 25 Mar 2019 18:35:42 +0100 Subject: [PATCH 1/2] fix(brushing): enable mouseup event outside chart element We now add the mouseup listener on the window element at the mousedown event and we remove it after the brush is end. We also disabled the tooltip when brushing. fix #119 --- .../react_canvas/reactive_chart.tsx | 4 +- src/state/chart_state.test.ts | 27 +++++++++++++ src/state/chart_state.ts | 40 ++++++++++++------- 3 files changed, 56 insertions(+), 15 deletions(-) diff --git a/src/components/react_canvas/reactive_chart.tsx b/src/components/react_canvas/reactive_chart.tsx index 00b994e8ad..e8442f00d8 100644 --- a/src/components/react_canvas/reactive_chart.tsx +++ b/src/components/react_canvas/reactive_chart.tsx @@ -193,6 +193,8 @@ class Chart extends React.Component { return ; } onStartBrusing = (event: { evt: MouseEvent }) => { + window.addEventListener('mouseup', this.onEndBrushing); + this.props.chartStore!.onBrushStart(); const { brushExtent } = this.props.chartStore!; const point = getPoint(event.evt, brushExtent); this.setState(() => ({ @@ -202,6 +204,7 @@ class Chart extends React.Component { })); } onEndBrushing = () => { + window.removeEventListener('mouseup', this.onEndBrushing); const { brushStart, brushEnd } = this.state; this.props.chartStore!.onBrushEnd(brushStart, brushEnd); this.setState(() => ({ @@ -254,7 +257,6 @@ class Chart extends React.Component { if (isBrushEnabled) { brushProps = { onMouseDown: this.onStartBrusing, - onMouseUp: this.onEndBrushing, onMouseMove: this.onBrushing, }; } diff --git a/src/state/chart_state.test.ts b/src/state/chart_state.test.ts index 880705a1a1..d5031a683c 100644 --- a/src/state/chart_state.test.ts +++ b/src/state/chart_state.test.ts @@ -3,6 +3,7 @@ import { DataSeriesColorsValues } from '../lib/series/series'; import { AxisSpec, BarSeriesSpec, Position } from '../lib/series/specs'; import { getAxisId, getGroupId, getSpecId } from '../lib/utils/ids'; import { TooltipType, TooltipValue } from '../lib/utils/interactions'; +import { ScaleContinuous } from '../lib/utils/scales/scale_continuous'; import { ScaleType } from '../lib/utils/scales/scales'; import { ChartStore } from './chart_state'; @@ -552,6 +553,32 @@ describe('Chart Store', () => { store.tooltipData.replace([tooltipValue]); expect(store.isTooltipVisible.get()).toBe(true); }); + + test('can disable tooltip on brushing', () => { + const tooltipValue: TooltipValue = { + name: 'a', + value: 'a', + color: 'a', + isHighlighted: false, + isXValue: false, + }; + store.xScale = new ScaleContinuous([0, 100], [0, 100], ScaleType.Linear); + store.cursorPosition.x = 1; + store.cursorPosition.x = 1; + store.tooltipType.set(TooltipType.Crosshairs); + store.tooltipData.replace([tooltipValue]); + store.onBrushStart(); + expect(store.isBrushing.get()).toBe(true); + expect(store.isTooltipVisible.get()).toBe(false); + + store.cursorPosition.x = 1; + store.cursorPosition.x = 1; + store.tooltipType.set(TooltipType.Crosshairs); + store.tooltipData.replace([tooltipValue]); + store.onBrushEnd({ x: 0, y: 0 }, { x: 1, y: 1 }); + expect(store.isBrushing.get()).toBe(false); + expect(store.isTooltipVisible.get()).toBe(true); + }); test('handle click on chart', () => { const geom1: IndexedGeometry = { color: 'red', diff --git a/src/state/chart_state.ts b/src/state/chart_state.ts index 5c5f8d8f63..a2d4579adf 100644 --- a/src/state/chart_state.ts +++ b/src/state/chart_state.ts @@ -117,6 +117,7 @@ export class ChartStore { y: 0, rotate: 0, }; + isBrushing = observable.box(false); brushExtent: BrushExtent = { minX: 0, minY: 0, @@ -385,6 +386,7 @@ export class ChartStore { isTooltipVisible = computed(() => { return ( + !this.isBrushing.get() && this.tooltipType.get() !== TooltipType.None && this.cursorPosition.x > -1 && this.cursorPosition.y > -1 && @@ -393,6 +395,7 @@ export class ChartStore { }); isCrosshairVisible = computed(() => { return ( + !this.isBrushing.get() && isCrosshairTooltipType(this.tooltipType.get()) && this.cursorPosition.x > -1 && this.cursorPosition.y > -1 @@ -521,6 +524,29 @@ export class ChartStore { } }); + onBrushStart = action(() => { + if (!this.onBrushEndListener) { + return; + } + this.isBrushing.set(true); + }); + + onBrushEnd = action((start: Point, end: Point) => { + if (!this.onBrushEndListener) { + return; + } + this.isBrushing.set(false); + const minValue = start.x < end.x ? start.x : end.x; + const maxValue = start.x > end.x ? start.x : end.x; + if (maxValue === minValue) { + // if 0 size brush, avoid computing the value + return; + } + const min = this.xScale!.invert(minValue - this.chartDimensions.left); + const max = this.xScale!.invert(maxValue - this.chartDimensions.left); + this.onBrushEndListener(min, max); + }); + handleChartClick() { if (this.highlightedGeometries.length > 0 && this.onElementClickListener) { this.onElementClickListener(this.highlightedGeometries.toJS()); @@ -579,20 +605,6 @@ export class ChartStore { removeOnLegendItemMinusClickListener() { this.onLegendItemMinusClickListener = undefined; } - onBrushEnd(start: Point, end: Point) { - if (!this.onBrushEndListener) { - return; - } - const minValue = start.x < end.x ? start.x : end.x; - const maxValue = start.x > end.x ? start.x : end.x; - if (maxValue === minValue) { - // if 0 size brush, avoid computing the value - return; - } - const min = this.xScale!.invert(minValue - this.chartDimensions.left); - const max = this.xScale!.invert(maxValue - this.chartDimensions.left); - this.onBrushEndListener(min, max); - } isBrushEnabled(): boolean { if (!this.xScale) { From b5e779041b228ca80dadb47c7453d0f958e290d1 Mon Sep 17 00:00:00 2001 From: Marco Vettorello Date: Tue, 26 Mar 2019 19:06:51 +0100 Subject: [PATCH 2/2] test: increase coverage on chart_state --- src/state/chart_state.test.ts | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/src/state/chart_state.test.ts b/src/state/chart_state.test.ts index d5031a683c..e6d3e63314 100644 --- a/src/state/chart_state.test.ts +++ b/src/state/chart_state.test.ts @@ -3,6 +3,7 @@ import { DataSeriesColorsValues } from '../lib/series/series'; import { AxisSpec, BarSeriesSpec, Position } from '../lib/series/specs'; import { getAxisId, getGroupId, getSpecId } from '../lib/utils/ids'; import { TooltipType, TooltipValue } from '../lib/utils/interactions'; +import { ScaleBand } from '../lib/utils/scales/scale_band'; import { ScaleContinuous } from '../lib/utils/scales/scale_continuous'; import { ScaleType } from '../lib/utils/scales/scales'; import { ChartStore } from './chart_state'; @@ -346,11 +347,14 @@ describe('Chart Store', () => { store.chartDimensions.left = 10; store.onBrushEndListener = undefined; + store.onBrushStart(); + expect(store.isBrushing.get()).toBe(false); store.onBrushEnd(start, end1); expect(brushEndListener).not.toBeCalled(); store.setOnBrushEndListener(brushEndListener); - + store.onBrushStart(); + expect(store.isBrushing.get()).toBe(true); store.onBrushEnd(start, start); expect(brushEndListener).not.toBeCalled(); @@ -363,13 +367,6 @@ describe('Chart Store', () => { expect(brushEndListener.mock.calls[1][1]).toBeCloseTo(0.9, 1); }); - test('can determine if brush is enabled', () => { - expect(store.isBrushEnabled()).toBe(true); - - store.xScale = undefined; - expect(store.isBrushEnabled()).toBe(false); - }); - test('can update parent dimensions', () => { const computeChart = jest.fn( (): void => { @@ -554,6 +551,18 @@ describe('Chart Store', () => { expect(store.isTooltipVisible.get()).toBe(true); }); + test('can disable brush based on scale and listener', () => { + store.xScale = undefined; + expect(store.isBrushEnabled()).toBe(false); + store.xScale = new ScaleContinuous([0, 100], [0, 100], ScaleType.Linear); + store.onBrushEndListener = undefined; + expect(store.isBrushEnabled()).toBe(false); + store.setOnBrushEndListener(() => ({})); + expect(store.isBrushEnabled()).toBe(true); + store.xScale = new ScaleBand([0, 100], [0, 100]); + expect(store.isBrushEnabled()).toBe(false); + }); + test('can disable tooltip on brushing', () => { const tooltipValue: TooltipValue = { name: 'a',