From 02eedcc63ed64084c907e61b65f359ae4a2bcbbe Mon Sep 17 00:00:00 2001 From: Marco Vettorello Date: Tue, 28 Jan 2020 18:17:21 +0100 Subject: [PATCH] fix(brush): rotate brush on rotated charts during the redux refactoring (#281) I've forgot to compute the brush correctly on rotated charts. In this commit I also fixed the min and max value passed to the onBrushEnd to be aligned with the min/max value of the domain. fix #527 --- .playground/index.html | 8 +- .playground/playground.tsx | 114 +++++++++--- .../xy_chart/renderer/dom/brush.tsx | 15 +- .../state/chart_state.interactions.test.ts | 173 +++++++++++++++++- .../xy_chart/state/chart_state.test.ts | 45 ----- .../state/selectors/get_brush_area.ts | 28 ++- .../state/selectors/on_brush_end_caller.ts | 20 +- src/state/reducers/interactions.ts | 2 +- 8 files changed, 295 insertions(+), 110 deletions(-) diff --git a/.playground/index.html b/.playground/index.html index 9345a20207..6d829f699f 100644 --- a/.playground/index.html +++ b/.playground/index.html @@ -29,11 +29,9 @@ /*display: inline-block; position: relative; */ - width: 100%; - height: 480px; - /* - margin: 0; -*/ + width: 800px; + height: 400px; + margin: 20px; } diff --git a/.playground/playground.tsx b/.playground/playground.tsx index b4edfad411..402d76f4bc 100644 --- a/.playground/playground.tsx +++ b/.playground/playground.tsx @@ -1,48 +1,112 @@ import React from 'react'; -import { Chart, Datum, Partition } from '../src'; +import { Chart, LineSeries, ScaleType, Settings, Position, Axis, BarSeries, HistogramBarSeries } from '../src'; export class Playground extends React.Component<{}, { isSunburstShown: boolean }> { chartRef: React.RefObject = React.createRef(); state = { isSunburstShown: true, }; + onBrushEnd = (min: number, max: number) => { + // eslint-disable-next-line no-console + console.log({ min, max }); + }; render() { return ( <>
- + + + + +
+
+ + + + + + +
+
+ + + + + d.v} - valueFormatter={(d: Datum) => `${d}%`} - layers={[ - { groupByRollup: (d: Datum) => d.g, nodeLabel: (d: Datum) => `Group ${d}` }, - { - groupByRollup: (d: Datum) => d.id, - nodeLabel: (d: Datum) => d, - fillLabel: { formatter: (d: Datum) => `${d} pct` }, - }, + /> +
- `${d}%`} - layers={[ - { - groupByRollup: (d: Datum, i: number) => (i < 2 ? 'A' : 'B'), - nodeLabel: (d: Datum) => `Group ${d}`, - }, - { groupByRollup: (d: Datum, i: number) => i, nodeLabel: (d: Datum) => `Item\u00A0${d}` }, + + + + diff --git a/src/chart_types/xy_chart/renderer/dom/brush.tsx b/src/chart_types/xy_chart/renderer/dom/brush.tsx index e2a01a47bf..42484297ea 100644 --- a/src/chart_types/xy_chart/renderer/dom/brush.tsx +++ b/src/chart_types/xy_chart/renderer/dom/brush.tsx @@ -3,8 +3,6 @@ import { Layer, Rect, Stage } from 'react-konva'; import { connect } from 'react-redux'; import { Dimensions } from '../../../../utils/dimensions'; import { isInitialized } from '../../../../state/selectors/is_initialized'; -import { computeChartTransformSelector } from '../../state/selectors/compute_chart_transform'; -import { Transform } from '../../state/utils'; import { GlobalChartState } from '../../../../state/chart_state'; import { getBrushAreaSelector } from '../../state/selectors/get_brush_area'; import { isBrushAvailableSelector } from '../../state/selectors/is_brush_available'; @@ -14,7 +12,6 @@ import { isBrushingSelector } from '../../state/selectors/is_brushing'; interface Props { initialized: boolean; chartDimensions: Dimensions; - chartTransform: Transform; isBrushing: boolean | undefined; isBrushAvailable: boolean | undefined; brushArea: Dimensions | null; @@ -32,7 +29,7 @@ class BrushToolComponent extends React.Component { }; render() { - const { initialized, isBrushAvailable, isBrushing, chartDimensions, chartTransform, brushArea } = this.props; + const { initialized, isBrushAvailable, isBrushing, chartDimensions, brushArea } = this.props; if (!initialized || !isBrushAvailable || !isBrushing) { return null; } @@ -43,8 +40,8 @@ class BrushToolComponent extends React.Component { height={chartDimensions.height} className="echBrushTool" style={{ - top: chartDimensions.top + chartTransform.x, - left: chartDimensions.left + chartTransform.y, + top: chartDimensions.top, + left: chartDimensions.left, width: chartDimensions.width, height: chartDimensions.height, }} @@ -70,11 +67,6 @@ const mapStateToProps = (state: GlobalChartState): Props => { width: 0, height: 0, }, - chartTransform: { - x: 0, - y: 0, - rotate: 0, - }, }; } return { @@ -82,7 +74,6 @@ const mapStateToProps = (state: GlobalChartState): Props => { brushArea: getBrushAreaSelector(state), isBrushAvailable: isBrushAvailableSelector(state), chartDimensions: computeChartDimensionsSelector(state).chartDimensions, - chartTransform: computeChartTransformSelector(state), isBrushing: isBrushingSelector(state), }; }; diff --git a/src/chart_types/xy_chart/state/chart_state.interactions.test.ts b/src/chart_types/xy_chart/state/chart_state.interactions.test.ts index f95b6aa831..f571811272 100644 --- a/src/chart_types/xy_chart/state/chart_state.interactions.test.ts +++ b/src/chart_types/xy_chart/state/chart_state.interactions.test.ts @@ -11,13 +11,14 @@ import { getTooltipValuesAndGeometriesSelector, } from './selectors/get_tooltip_values_highlighted_geoms'; import { isTooltipVisibleSelector } from './selectors/is_tooltip_visible'; +import { createOnBrushEndCaller } from './selectors/on_brush_end_caller'; import { createOnElementOutCaller } from './selectors/on_element_out_caller'; import { createOnElementOverCaller } from './selectors/on_element_over_caller'; import { getCursorBandPositionSelector } from './selectors/get_cursor_band'; import { getSettingsSpecSelector } from '../../../state/selectors/get_settings_specs'; import { upsertSpec, specParsed } from '../../../state/actions/specs'; import { updateParentDimensions } from '../../../state/actions/chart_settings'; -import { onPointerMove } from '../../../state/actions/mouse'; +import { onPointerMove, onMouseDown, onMouseUp } from '../../../state/actions/mouse'; import { ChartTypes } from '../..'; import { createOnPointerMoveCaller } from './selectors/on_pointer_move_caller'; import { onExternalPointerEvent } from '../../../state/actions/events'; @@ -707,4 +708,174 @@ function mouseOverTestSuite(scaleType: ScaleType) { expect(tooltipData.tooltipValues[1].value).toBe('bottom 5'); }); }); + describe('brush', () => { + test('can respond to a brush end event', () => { + const brushEndListener = jest.fn((): void => { + return; + }); + const onBrushCaller = createOnBrushEndCaller(); + store.subscribe(() => { + onBrushCaller(store.getState()); + }); + const settings = getSettingsSpecSelector(store.getState()); + const updatedSettings: SettingsSpec = { + ...settings, + theme: { + ...settings.theme, + chartMargins: { + left: 0, + right: 0, + top: 0, + bottom: 0, + }, + }, + onBrushEnd: brushEndListener, + }; + store.dispatch(upsertSpec(updatedSettings)); + store.dispatch( + upsertSpec({ + ...spec, + data: [ + [0, 1], + [1, 1], + [2, 2], + [3, 3], + ], + } as BarSeriesSpec), + ); + store.dispatch(specParsed()); + + const start1 = { x: 0, y: 0 }; + const end1 = { x: 75, y: 0 }; + + store.dispatch(onMouseDown(start1, 0)); + store.dispatch(onPointerMove(end1, 1)); + store.dispatch(onMouseUp(end1, 3)); + if (scaleType === ScaleType.Ordinal) { + expect(brushEndListener).not.toBeCalled(); + } else { + expect(brushEndListener).toBeCalled(); + expect(brushEndListener.mock.calls[0][0]).toBe(0); + expect(brushEndListener.mock.calls[0][1]).toBe(2.5); + } + const start2 = { x: 75, y: 0 }; + const end2 = { x: 100, y: 0 }; + + store.dispatch(onMouseDown(start2, 4)); + store.dispatch(onPointerMove(end2, 5)); + store.dispatch(onMouseUp(end2, 6)); + if (scaleType === ScaleType.Ordinal) { + expect(brushEndListener).not.toBeCalled(); + } else { + expect(brushEndListener).toBeCalled(); + expect(brushEndListener.mock.calls[1][0]).toBe(2.5); + expect(brushEndListener.mock.calls[1][1]).toBe(3); + } + + const start3 = { x: 75, y: 0 }; + const end3 = { x: 250, y: 0 }; + store.dispatch(onMouseDown(start3, 7)); + store.dispatch(onPointerMove(end3, 8)); + store.dispatch(onMouseUp(end3, 9)); + if (scaleType === ScaleType.Ordinal) { + expect(brushEndListener).not.toBeCalled(); + } else { + expect(brushEndListener).toBeCalled(); + expect(brushEndListener.mock.calls[2][0]).toBe(2.5); + expect(brushEndListener.mock.calls[2][1]).toBe(3); + } + + const start4 = { x: 25, y: 0 }; + const end4 = { x: -20, y: 0 }; + store.dispatch(onMouseDown(start4, 10)); + store.dispatch(onPointerMove(end4, 11)); + store.dispatch(onMouseUp(end4, 12)); + if (scaleType === ScaleType.Ordinal) { + expect(brushEndListener).not.toBeCalled(); + } else { + expect(brushEndListener).toBeCalled(); + expect(brushEndListener.mock.calls[3][0]).toBe(0); + expect(brushEndListener.mock.calls[3][1]).toBe(0.5); + } + }); + test('can respond to a brush end event on rotated chart', () => { + const brushEndListener = jest.fn((): void => { + return; + }); + const onBrushCaller = createOnBrushEndCaller(); + store.subscribe(() => { + onBrushCaller(store.getState()); + }); + const settings = getSettingsSpecSelector(store.getState()); + const updatedSettings: SettingsSpec = { + ...settings, + rotation: 90, + theme: { + ...settings.theme, + chartMargins: { + left: 0, + right: 0, + top: 0, + bottom: 0, + }, + }, + onBrushEnd: brushEndListener, + }; + store.dispatch(upsertSpec(updatedSettings)); + store.dispatch(specParsed()); + + const start1 = { x: 0, y: 25 }; + const end1 = { x: 0, y: 75 }; + + store.dispatch(onMouseDown(start1, 0)); + store.dispatch(onPointerMove(end1, 1)); + store.dispatch(onMouseUp(end1, 3)); + if (scaleType === ScaleType.Ordinal) { + expect(brushEndListener).not.toBeCalled(); + } else { + expect(brushEndListener).toBeCalled(); + expect(brushEndListener.mock.calls[0][0]).toBe(0); + expect(brushEndListener.mock.calls[0][1]).toBe(1); + } + const start2 = { x: 0, y: 75 }; + const end2 = { x: 0, y: 100 }; + + store.dispatch(onMouseDown(start2, 4)); + store.dispatch(onPointerMove(end2, 5)); + store.dispatch(onMouseUp(end2, 6)); + if (scaleType === ScaleType.Ordinal) { + expect(brushEndListener).not.toBeCalled(); + } else { + expect(brushEndListener).toBeCalled(); + expect(brushEndListener.mock.calls[1][0]).toBe(1); + expect(brushEndListener.mock.calls[1][1]).toBe(1); + } + + const start3 = { x: 0, y: 75 }; + const end3 = { x: 0, y: 200 }; + store.dispatch(onMouseDown(start3, 7)); + store.dispatch(onPointerMove(end3, 8)); + store.dispatch(onMouseUp(end3, 9)); + if (scaleType === ScaleType.Ordinal) { + expect(brushEndListener).not.toBeCalled(); + } else { + expect(brushEndListener).toBeCalled(); + expect(brushEndListener.mock.calls[2][0]).toBe(1); + expect(brushEndListener.mock.calls[2][1]).toBe(1); // max of chart + } + + const start4 = { x: 0, y: 25 }; + const end4 = { x: 0, y: -20 }; + store.dispatch(onMouseDown(start4, 10)); + store.dispatch(onPointerMove(end4, 11)); + store.dispatch(onMouseUp(end4, 12)); + if (scaleType === ScaleType.Ordinal) { + expect(brushEndListener).not.toBeCalled(); + } else { + expect(brushEndListener).toBeCalled(); + expect(brushEndListener.mock.calls[3][0]).toBe(0); + expect(brushEndListener.mock.calls[3][1]).toBe(0); + } + }); + }); } diff --git a/src/chart_types/xy_chart/state/chart_state.test.ts b/src/chart_types/xy_chart/state/chart_state.test.ts index 0b481a88e5..6a603c306c 100644 --- a/src/chart_types/xy_chart/state/chart_state.test.ts +++ b/src/chart_types/xy_chart/state/chart_state.test.ts @@ -8,8 +8,6 @@ import { RectAnnotationSpec, SeriesTypes, } from '../utils/specs'; -import { LIGHT_THEME } from '../../../utils/themes/light_theme'; -import { mergeWithDefaultTheme } from '../../../utils/themes/theme'; import { TooltipType, TooltipValue } from '../utils/interactions'; import { ScaleBand } from '../../../utils/scales/scale_band'; import { ScaleContinuous } from '../../../utils/scales/scale_continuous'; @@ -579,49 +577,6 @@ describe.skip('Chart Store', () => { expect(store.onRenderChangeListener).toBeUndefined(); }); - test.skip('can respond to a brush end event', () => { - const brushEndListener = jest.fn((): void => { - return; - }); - - const start1 = { x: 0, y: 0 }; - const start2 = { x: 100, y: 0 }; - const end1 = { x: 600, y: 0 }; - const end2 = { x: 300, y: 0 }; - store.chartTheme = mergeWithDefaultTheme( - { - chartMargins: { - left: 0, - right: 0, - top: 0, - bottom: 0, - }, - }, - LIGHT_THEME, - ); - store.addSeriesSpec(spec); - store.computeChart(); - store.onBrushEndListener = undefined; - store.onBrushStart(); - expect(store.isBrushing.get()).toBe(false); - store.onBrushEnd(start1, end1); - expect(brushEndListener).not.toBeCalled(); - - store.setOnBrushEndListener(brushEndListener); - store.onBrushStart(); - expect(store.isBrushing.get()).toBe(true); - store.onBrushEnd(start1, start1); - expect(brushEndListener).not.toBeCalled(); - - store.onBrushEnd(start1, end1); - expect(brushEndListener.mock.calls[0][0]).toBe(1); - expect(brushEndListener.mock.calls[0][1]).toBe(4); - - store.onBrushEnd(start2, end2); - expect(brushEndListener.mock.calls[1][0]).toBe(1.5); - expect(brushEndListener.mock.calls[1][1]).toBe(2.5); - }); - test.skip('can update parent dimensions', () => { const computeChart = jest.fn((): void => { return; diff --git a/src/chart_types/xy_chart/state/selectors/get_brush_area.ts b/src/chart_types/xy_chart/state/selectors/get_brush_area.ts index 2d97a0ee0e..c919a1d668 100644 --- a/src/chart_types/xy_chart/state/selectors/get_brush_area.ts +++ b/src/chart_types/xy_chart/state/selectors/get_brush_area.ts @@ -1,7 +1,6 @@ import createCachedSelector from 're-reselect'; import { GlobalChartState } from '../../../../state/chart_state'; import { Dimensions } from '../../../../utils/dimensions'; -import { computeChartTransformSelector } from './compute_chart_transform'; import { getChartRotationSelector } from '../../../../state/selectors/get_chart_rotation'; import { computeChartDimensionsSelector } from './compute_chart_dimensions'; import { getChartIdSelector } from '../../../../state/selectors/get_chart_id'; @@ -12,36 +11,31 @@ const getCurrentPointerPosition = (state: GlobalChartState) => { }; export const getBrushAreaSelector = createCachedSelector( - [ - getMouseDownPosition, - getCurrentPointerPosition, - getChartRotationSelector, - computeChartDimensionsSelector, - computeChartTransformSelector, - ], - (mouseDownPosition, cursorPosition, chartRotation, { chartDimensions }, chartTransform): Dimensions | null => { + [getMouseDownPosition, getCurrentPointerPosition, getChartRotationSelector, computeChartDimensionsSelector], + (mouseDownPosition, cursorPosition, chartRotation, { chartDimensions }): Dimensions | null => { if (!mouseDownPosition) { return null; } const brushStart = { - x: mouseDownPosition.position.x - chartDimensions.left, - y: mouseDownPosition.position.y - chartDimensions.top, + x: mouseDownPosition.position.x, + y: mouseDownPosition.position.y, }; if (chartRotation === 0 || chartRotation === 180) { const area = { - left: brushStart.x, + left: brushStart.x - chartDimensions.left, + width: cursorPosition.x - brushStart.x, top: 0, - width: cursorPosition.x - brushStart.x - chartDimensions.left, height: chartDimensions.height, }; return area; } else { - return { - left: chartDimensions.left + chartTransform.x, - top: brushStart.y - chartDimensions.top, + const area = { + left: 0, width: chartDimensions.width, - height: cursorPosition.y - brushStart.y - chartDimensions.top, + top: brushStart.y - chartDimensions.top, + height: cursorPosition.y - brushStart.y, }; + return area; } }, )(getChartIdSelector); diff --git a/src/chart_types/xy_chart/state/selectors/on_brush_end_caller.ts b/src/chart_types/xy_chart/state/selectors/on_brush_end_caller.ts index b2bb04e192..7725c91326 100644 --- a/src/chart_types/xy_chart/state/selectors/on_brush_end_caller.ts +++ b/src/chart_types/xy_chart/state/selectors/on_brush_end_caller.ts @@ -66,8 +66,16 @@ export function createOnBrushEndCaller(): (state: GlobalChartState) => void { if (lastDrag !== null && hasDragged(prevProps, nextProps)) { if (settings && settings.onBrushEnd) { - const minValue = Math.min(lastDrag.start.position.x, lastDrag.end.position.x); - const maxValue = Math.max(lastDrag.start.position.x, lastDrag.end.position.x); + let startPos = lastDrag.start.position.x - chartDimensions.left; + let endPos = lastDrag.end.position.x - chartDimensions.left; + let chartMax = chartDimensions.width; + if (settings.rotation === -90 || settings.rotation === 90) { + startPos = lastDrag.start.position.y - chartDimensions.top; + endPos = lastDrag.end.position.y - chartDimensions.top; + chartMax = chartDimensions.height; + } + const minValue = Math.max(Math.min(startPos, endPos), 0); + const maxValue = Math.min(Math.max(startPos, endPos), chartMax); if (maxValue === minValue) { // if 0 size brush, avoid computing the value return; @@ -75,8 +83,12 @@ export function createOnBrushEndCaller(): (state: GlobalChartState) => void { const { xScale } = computedScales; const offset = histogramMode ? 0 : -(xScale.bandwidth + xScale.bandwidthPadding) / 2; - const min = xScale.invert(minValue - chartDimensions.left + offset); - const max = xScale.invert(maxValue - chartDimensions.left + offset); + const min = Math.max(xScale.invert(minValue + offset), xScale.domain[0]); + + const max = Math.min( + xScale.invert(maxValue + offset), + histogramMode ? xScale.domain[1] + xScale.bandwidth + xScale.bandwidthPadding : xScale.domain[1], + ); settings.onBrushEnd(min, max); } } diff --git a/src/state/reducers/interactions.ts b/src/state/reducers/interactions.ts index 3a5a9ccfd2..203ade3392 100644 --- a/src/state/reducers/interactions.ts +++ b/src/state/reducers/interactions.ts @@ -56,7 +56,7 @@ export function interactionsReducer(state: InteractionsState, action: LegendActi }, end: { position: { - ...action.position, + ...state.pointer.current.position, }, time: action.time, },