Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(heatmap): remove values when brushing only over axes #1364

Merged
merged 10 commits into from
Sep 13, 2021
Original file line number Diff line number Diff line change
Expand Up @@ -39,11 +39,10 @@ export const HighlighterCellsComponent: FC<HighlighterCellsProps> = ({
}) => {
if (!initialized || dragShape === null) return null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also a preexisting code improvement opportunity: Here, we yield null if these predicates are true. But the continuation branch also yields null if pointerOnLeftAxis is true. So the bail can be deleted like this:

}) => {
  const pointerOnLeftAxis = (pointer?.lastDrag?.start.position.x ?? Infinity) < canvasDimension.left;
  return dragShape === null || !initialized || pointerOnLeftAxis ? null : (

A possible objection is that maybe it's stupid to calculate pointerOnLeftAxis if eg. dragShape === null, its value won't be used. However, the way current JS engines work, it likely won't be computed, as JS engines are free to reorder operations, so most likely, it won't evaluate the pointerOnLeftAxis expression unless needed so. It could be a different situation if the pointerOnLeftAxis expression called some expensive function.

Also, it's such a cheap calculation even if it were done, performance is not a concern, any difference is unmeasurable.

The gain is not just fewer lines to read (us) and send to the browser (payload) but the simpler, linear start to finish flow. Unless there's a strong reason, it's nice to work with expressions rather than statements, and it's nice to have a singular return point


const maskId = `echHighlighterMask__${chartId}`;
return (
<svg className="echHighlighter" width="100%" height="100%">
<defs>
<mask id={maskId}>
<mask id={`echHighlighterMask__${chartId}`}>
{/* the entire chart */}
{brushMask.visible && (
<rect
Expand Down Expand Up @@ -84,7 +83,7 @@ export const HighlighterCellsComponent: FC<HighlighterCellsProps> = ({
y={0}
width={canvasDimension.width + canvasDimension.left}
height={canvasDimension.height}
mask={`url(#${maskId})`}
mask={`url(#echHighlighterMask__${chartId})`}
fill={brushMask.fill}
/>
)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ export const getBrushAreaSelector = createCustomCachedSelector(
if (!isDragging || !mouseDownPosition || !dragShape) {
return null;
}

const start = {
x: mouseDownPosition.position.x - chartDimensions.left,
y: mouseDownPosition.position.y,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License
* 2.0 and the Server Side Public License, v 1; you may not use this file except
* in compliance with, at your election, the Elastic License 2.0 or the Server
* Side Public License, v 1.
*/

import { Store } from 'redux';

import { MockGlobalSpec, MockSeriesSpec } from '../../../../mocks/specs/specs';
import { MockStore } from '../../../../mocks/store/store';
import { ScaleType } from '../../../../scales/constants';
import { onPointerMove, onMouseDown, onMouseUp } from '../../../../state/actions/mouse';
import { GlobalChartState } from '../../../../state/chart_state';
import { createOnBrushEndCaller } from './on_brush_end_caller';

describe('Heatmap picked cells', () => {
let store: Store<GlobalChartState>;
let onBrushEndMock = jest.fn();

beforeEach(() => {
store = MockStore.default({ width: 300, height: 300, top: 0, left: 0 }, 'chartId');
onBrushEndMock = jest.fn();
MockStore.addSpecs(
[
MockGlobalSpec.settingsNoMargins(),
MockSeriesSpec.heatmap({
xScaleType: ScaleType.Ordinal,
data: [
{ x: 'a', y: 'ya', value: 1 },
{ x: 'b', y: 'ya', value: 2 },
{ x: 'c', y: 'ya', value: 3 },
{ x: 'a', y: 'yb', value: 4 },
{ x: 'b', y: 'yb', value: 5 },
{ x: 'c', y: 'yb', value: 6 },
{ x: 'a', y: 'yc', value: 7 },
{ x: 'b', y: 'yc', value: 8 },
{ x: 'c', y: 'yc', value: 9 },
],
config: {
brushTool: { visible: true },
grid: {
cellHeight: {
max: 'fill',
},
cellWidth: {
max: 'fill',
},
},
xAxisLabel: {
visible: true,
},
yAxisLabel: {
visible: true,
},
margin: { top: 0, bottom: 0, left: 0, right: 0 },
onBrushEnd: onBrushEndMock,
},
}),
],
store,
);
});

it('should pick cells', () => {
const caller = createOnBrushEndCaller();
store.dispatch(onPointerMove({ x: 50, y: 50 }, 0));
store.dispatch(onMouseDown({ x: 50, y: 50 }, 100));
store.dispatch(onPointerMove({ x: 150, y: 250 }, 200));
store.dispatch(onMouseUp({ x: 150, y: 250 }, 300));
caller(store.getState());
const brushEvent = onBrushEndMock.mock.calls[0][0];
expect(brushEvent.x.length).toBe(2);
});
it('should not include x values if only dragging along y-axis', () => {
const caller = createOnBrushEndCaller();
store.dispatch(onPointerMove({ x: 0, y: 50 }, 0));
store.dispatch(onMouseDown({ x: 0, y: 50 }, 100));
store.dispatch(onPointerMove({ x: 0, y: 20 }, 200));
store.dispatch(onMouseUp({ x: 0, y: 20 }, 300));
caller(store.getState());
const brushEvent = onBrushEndMock.mock.calls[0][0];
expect(brushEvent.x.length).toBe(0);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,34 @@
import { createCustomCachedSelector } from '../../../../state/create_selector';
import { getLastDragSelector } from '../../../../state/selectors/get_last_drag';
import { PickDragFunction } from '../../layout/types/viewmodel_types';
import { computeChartDimensionsSelector } from './compute_chart_dimensions';
import { geometries } from './geometries';
import { getGridHeightParamsSelector } from './get_grid_full_height';

/** @internal */
export const getPickedCells = createCustomCachedSelector(
[geometries, getLastDragSelector],
(geoms, dragState): ReturnType<PickDragFunction> | null => {
[geometries, getLastDragSelector, computeChartDimensionsSelector, getGridHeightParamsSelector],
(geoms, dragState, canvasDimensions, gridParams): ReturnType<PickDragFunction> | null => {
if (!dragState) {
return null;
}

// the pointer is not on the cells but over the y- axis and does not cross the y-axis
if (dragState.start.position.x < canvasDimensions.left && dragState.end.position.x < canvasDimensions.left) {
const fittedDragStateStart = { x: canvasDimensions.left, y: dragState.start.position.y };
const { y, cells } = geoms.pickDragArea([fittedDragStateStart, dragState.end.position]);
return { x: [], y, cells };
}

// the pointer is not on the cells by over the x-axis and does not cross the x-axis
if (
dragState.start.position.y > gridParams.gridCellHeight * gridParams.pageSize &&
dragState.end.position.y > gridParams.gridCellHeight * gridParams.pageSize
) {
const fittedDragStateStart = { x: dragState.start.position.x, y: canvasDimensions.height };
const { x, cells } = geoms.pickDragArea([fittedDragStateStart, dragState.end.position]);
return { x, y: [], cells };
}
return geoms.pickDragArea([dragState.start.position, dragState.end.position]);
},
);