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

rshen91 · 2021-09-07T21:44:48Z

Summary

If a user brushes only over the y axis, x values should not be calculated in the pickedCells. If a user brushes only over the x axis, the y values should not be populated.

Details

This PR removes x values in picked cells when brushing only over y-axis and remove y values in picked cells when brushing over the x axis.

Issues

Closes #1338

Checklist

The proper chart type label has been added (e.g. :xy, :partition)
All related issues have been linked (i.e. closes #123, fixes #123)
Unit tests have been added or updated to match the most common scenarios
The proper documentation and/or storybook story has been added or updated

markov00 · 2021-09-08T07:57:13Z

@rshen91 can you please add a link to an issue for this, or describe it here?
I'm not sure what are you talking about, but if the reason is to disable brushing over the Y axis I think this is not desirable, or at least not from the ML team

rshen91 · 2021-09-08T14:52:14Z

Hi @markov00 sorry I'm still trying to figure out implementation for this. I think I'm following that brushing should be disabled when the pointer starts at a position over the y axis. The issue I'm trying to fix is #1338, thanks

monfera · 2021-09-09T07:51:59Z

packages/charts/src/chart_types/heatmap/renderer/dom/highlighter.tsx

-  return (
+  const pointerOnLeftAxis =
+    pointer && pointer.lastDrag ? pointer.lastDrag?.start.position.x < canvasDimension.left : false;
+  return !pointerOnLeftAxis ? (


Nice ternary here! I try not to negate a singular predicate in such cases, no real issue with null being in the 1st outcome of the ternary, in fact a bit nicer as it gets the non-result out of the way and the code reader can focus on the DOM fragment

monfera · 2021-09-09T08:13:41Z

packages/charts/src/chart_types/heatmap/renderer/dom/highlighter.tsx

+  const pointerOnLeftAxis =
+    pointer && pointer.lastDrag ? pointer.lastDrag?.start.position.x < canvasDimension.left : false;


When there's a true or false as a value expressly yielded eg. via a ternary, it almost always means, it can be simplified:

const pointerOnLeftAxis = pointer && pointer.lastDrag && pointer.lastDrag?.start.position.x < canvasDimension.left;

Explanation: if pointer or pointer.lastDrag are falsey, the expression evaluates to falsey anyway.

The question mark after the second use of lastDrag isn't needed, because TS will type guard it anyway, since you never land on that if pointer.lastDrag is nullish:

const pointerOnLeftAxis = pointer && pointer.lastDrag && pointer.lastDrag.start.position.x < canvasDimension.left;

Also, pointer && pointer.lastDrag can usually be collapsed like this:

const pointerOnLeftAxis = pointer?.lastDrag && pointer.lastDrag.start.position.x < canvasDimension.left;

It still has some repetition: the pointer.lastDrag chain. We can NOT simply say,

const pointerOnLeftAxis = pointer?.lastDrag?.start.position.x < canvasDimension.left;

in part because TypeScript prevents us from doing automatic type coercion, so only a number can be < compared with another number (or eg. string with string); in part because a nullish comparison in JS may yield unexpected results, which is why TS prevents coercion to begin with (eg. undefined < 3 === false but null < 3 is true, as null gets coerced to zero (null == 0 is false though...)

However we can use a value for nullish coalescing, which will be always greater than canvasDimension.left, avoiding the above repetition as well as the ternary:

Suggested change

const pointerOnLeftAxis =

pointer && pointer.lastDrag ? pointer.lastDrag?.start.position.x < canvasDimension.left : false;

const pointerOnLeftAxis = (pointer?.lastDrag?.start.position.x ?? Infinity) < canvasDimension.left;

(Caveats, I haven't tried these TS-wise or runtime-wise, pls. mention if it doesn't work out; also, I'm not 100% sure if it's a < or <= predicate that's better here)

monfera · 2021-09-09T08:16:01Z

packages/charts/src/chart_types/heatmap/renderer/dom/highlighter.tsx

@@ -36,11 +38,14 @@ export const HighlighterCellsComponent: FC<HighlighterCellsProps> = ({
  canvasDimension,
  brushArea,
  brushMask,
+  pointer,
 }) => {
  if (!initialized || dragShape === null) return null;

  const maskId = `echHighlighterMask__${chartId}`;


While in the neighborhood, I'd inline maskId in the preexisting code even if it's not your addition in this PR: maskId has a single use, not reused; its meaning is clear already at that place of use, (<mask id={...}> is clearly a mask id; the variable name doesn't add info); and no need to precompute it as we may still yield null; one fewer line, and clarity about no multiple-use variable. So, the suggestion is, <mask id={echHighlighterMask__${chartId}}>

Sounds good to me - small fix in e5ca9d9 thanks

monfera · 2021-09-09T08:30:16Z

packages/charts/src/chart_types/heatmap/renderer/dom/highlighter.tsx

@@ -36,11 +38,14 @@ export const HighlighterCellsComponent: FC<HighlighterCellsProps> = ({
  canvasDimension,
  brushArea,
  brushMask,
+  pointer,
 }) => {
  if (!initialized || dragShape === null) return null;


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

monfera · 2021-09-09T08:44:43Z

packages/charts/src/chart_types/heatmap/renderer/dom/highlighter.tsx

@@ -22,6 +23,7 @@ export interface HighlighterCellsProps {
  dragShape: DragShape | null;
  brushMask: Config['brushMask'];
  brushArea: Config['brushArea'];
+  pointer: PointerStates | null;


Let's avoid adding a union type: an initial value, currently defined as null, can be replaced with something like

pointer = { dragging: false, current: { position: { x: NaN, y: NaN}, time: -Infinity }, down: null, up: null, lastDrag: null, lastClick: null }

Yes it's a bit verbose, but then the const pointerOnLeftAxis = ... predicate can further simplify (there's always a pointer object, etc.)

Having said that, the entirety of PointerStates, or its props except the first 1 or 2 props could be profitably replaced with a Map, as it is already a Map in spirit. No need for all these ternaries. Or PointerStates props could be retained as they are, except removing the | null ternary (initial values are trivial) or at least, not needing a null (can be an optional value, left undefined if unused)

@markov00 do you agree with such eventual refactoring, and in the scope of this PR, at least the avoidance of yet another union type?

rshen91 · 2021-09-09T15:29:01Z

@markov00 and @monfera thanks for the initial review I think I'm understanding the solution better 😅 in commit 7c131ed
@monfera I removed the older code you had commented on since I was misunderstanding the problem sorry for the hassle in reviewing!

monfera

The code looks good and the PR addresses the second point of the issue: the raised event is not specific to X values.
As it's a clear impovement, and helps the requestor test the changes, I think it's a positive step, so I'm approving it.

The first half of the issue is not addressed by this PR right now: the highlighting only highlighs the first column, rather than the entire width of the highlighted rows. So please remove the "closes [issue number]" to avoid closing the issue if it gets merged without this half

Also, while the issue was filed with reference to the Y axis, it should be the same behavior for the X axis too, otherwise users will be confused.

Another observation, may or may not be a Storybook artifact: after I click on the Actions data on the right, I'm sometimes unable to highlight the approx. same area again, unless I highlight something else. It may be a bug, as it seems to happen if I highlight within the area of a previous highlight

So it means there'd be two more tasks for full closure of the issue, as well as the filing of a bug report for this selection glitch, if needed

monfera

The code looks good and the PR addresses the second point of the issue: the raised event is not specific to X values.
As it's a clear impovement, and helps the requestor test the changes, I think it's a positive step, so I'm approving it.

The first half of the issue is not addressed by this PR right now: the highlighting only highlighs the first column, rather than the entire width of the highlighted rows. So please remove the "closes [issue number]" to avoid closing the issue if it gets merged without this half

Also, while the issue was filed with reference to the Y axis, it should be the same behavior for the X axis too, otherwise users will be confused.

Another observation, may or may not be a Storybook artifact: after I click on the Actions data on the right, I'm sometimes unable to highlight the approx. same area again, unless I highlight something else. It may be a bug, as it seems to happen if I highlight within the area of a previous highlight

So it means there'd be two more tasks for full closure of the issue, as well as the filing of a bug report for this selection glitch, if needed

rshen91 · 2021-09-10T17:08:14Z

@monfera good points. I added 11a84ec to add the logic to the x-axis dragging. Thanks!

markov00 · 2021-09-10T21:32:29Z

nit: please reduce and simplify the title length, and move it to the description

# [35.0.0](v34.2.1...v35.0.0) (2021-09-13) ### Bug Fixes * **a11y:** restore focus after popover close with color picker ([#1272](#1272)) ([0c6f945](0c6f945)), closes [#1266](#1266) [#935](#935) * **build:** fix license in package.json ([#1362](#1362)) ([d524fdf](d524fdf)) * **deps:** update dependency @elastic/eui to ^37.5.0 ([#1341](#1341)) ([fb05c98](fb05c98)) * **deps:** update dependency @elastic/eui to ^37.6.1 ([#1359](#1359)) ([2ae90ce](2ae90ce)) * **deps:** update dependency @elastic/eui to ^37.7.0 ([#1373](#1373)) ([553b6b0](553b6b0)) * **heatmap:** filter out tooltip picked shapes in x-axis area ([#1351](#1351)) ([174047d](174047d)), closes [#1215](#1215) * **heatmap:** remove values when brushing only over axes ([#1364](#1364)) ([77ff8d3](77ff8d3)) ### Features * **annotations:** add onClickHandler for annotations ([#1293](#1293)) ([48198be](48198be)), closes [#1211](#1211) * **heatmap:** add text color contrast to heatmap cells ([#1342](#1342)) ([f9a26ef](f9a26ef)), closes [#1296](#1296) * **heatmap:** reduce font size to fit label within cells ([#1352](#1352)) ([16b5546](16b5546)) * **xy:** mutilayer time axis step 1 ([#1326](#1326)) ([867b1f5](867b1f5)) ### BREAKING CHANGES * **xy:** - feat: removes the axis deduplication feature - fix: `showDuplicatedTicks` causes a duplication check on the actual axis tick label (possibly yielded by `Axis.tickLabel` rather than the more general `tickFormat`) * **heatmap:** the `config.label.fontSize` prop is replaced by `config.label.minFontSize` and `config.label.maxFontSize`. You can specify the same value for both properties to have a fixed font size. The `config.label.align` and `config.label.baseline` props are removed from the `HeatmapConfig` object.

nickofthyme · 2021-09-13T15:57:45Z

🎉 This PR is included in version 35.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

rshen91 added 2 commits September 7, 2021 15:43

feat: crude attempt for limiting brush

67a5811

Merge remote-tracking branch 'upstream/master' into heatmap-brush-y

639a050

Merge remote-tracking branch 'upstream/master' into heatmap-brush-y

51fce1b

rshen91 added the wip work in progress label Sep 8, 2021

monfera reviewed Sep 9, 2021

View reviewed changes

feat: remove x values from pickedShapes onbrushend event

7c131ed

rshen91 changed the title ~~fix(heatmap): refuse brush over y-axis~~ fix(heatmap): remove x values in picked cells when brushing only over y-axis Sep 9, 2021

rshen91 removed the wip work in progress label Sep 9, 2021

rshen91 requested a review from monfera September 9, 2021 15:41

rshen91 added 2 commits September 9, 2021 11:27

test: add unit test

cd4b51a

style: inline mask id

e5ca9d9

rshen91 marked this pull request as ready for review September 9, 2021 17:58

rshen91 requested a review from markov00 September 9, 2021 19:16

Merge remote-tracking branch 'upstream/master' into heatmap-brush-y

d6e1ca2

monfera suggested changes Sep 10, 2021

View reviewed changes

monfera self-requested a review September 10, 2021 16:41

monfera approved these changes Sep 10, 2021

View reviewed changes

feat: remove y values from x axis brushing

11a84ec

rshen91 changed the title ~~fix(heatmap): remove x values in picked cells when brushing only over y-axis~~ fix(heatmap): remove x values in picked cells when brushing only over y-axis and remove y values in picked cells when brushing over the x axis Sep 10, 2021

fix: fix height calculation for the x axis hovering

d3aa056

nickofthyme force-pushed the master branch from 8af17cf to 6dd67fb Compare September 13, 2021 03:33

nickofthyme force-pushed the master branch 6 times, most recently from 7148acf to 604b153 Compare September 13, 2021 04:06

rshen91 changed the title ~~fix(heatmap): remove x values in picked cells when brushing only over y-axis and remove y values in picked cells when brushing over the x axis~~ fix(heatmap): remove values when brushing only over axes Sep 13, 2021

Merge remote-tracking branch 'upstream/master' into heatmap-brush-y

fb0b2db

rshen91 merged commit 77ff8d3 into elastic:master Sep 13, 2021

rshen91 deleted the heatmap-brush-y branch September 13, 2021 14:49

nickofthyme added the released Issue released publicly label Sep 13, 2021

markov00 mentioned this pull request Oct 5, 2021

Update dependency @elastic/charts to v37 (master) elastic/kibana#113968

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rshen91 commented Sep 7, 2021 •

edited

Loading

markov00 commented Sep 8, 2021

rshen91 commented Sep 8, 2021

monfera Sep 9, 2021

monfera Sep 9, 2021

monfera Sep 9, 2021 •

edited

Loading

rshen91 Sep 9, 2021

monfera Sep 9, 2021

monfera Sep 9, 2021 •

edited

Loading

rshen91 commented Sep 9, 2021

monfera left a comment •

edited

Loading

monfera left a comment •

edited

Loading

rshen91 commented Sep 10, 2021

markov00 commented Sep 10, 2021

nickofthyme commented Sep 13, 2021

		const pointerOnLeftAxis =
		pointer && pointer.lastDrag ? pointer.lastDrag?.start.position.x < canvasDimension.left : false;

	const pointerOnLeftAxis =
	pointer && pointer.lastDrag ? pointer.lastDrag?.start.position.x < canvasDimension.left : false;
	const pointerOnLeftAxis = (pointer?.lastDrag?.start.position.x ?? Infinity) < canvasDimension.left;

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

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

Conversation

rshen91 commented Sep 7, 2021 • edited Loading

Summary

Details

Issues

Checklist

markov00 commented Sep 8, 2021

rshen91 commented Sep 8, 2021

monfera Sep 9, 2021

Choose a reason for hiding this comment

monfera Sep 9, 2021

Choose a reason for hiding this comment

monfera Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

rshen91 Sep 9, 2021

Choose a reason for hiding this comment

monfera Sep 9, 2021

Choose a reason for hiding this comment

monfera Sep 9, 2021 • edited Loading

Choose a reason for hiding this comment

rshen91 commented Sep 9, 2021

monfera left a comment • edited Loading

Choose a reason for hiding this comment

monfera left a comment • edited Loading

Choose a reason for hiding this comment

rshen91 commented Sep 10, 2021

markov00 commented Sep 10, 2021

nickofthyme commented Sep 13, 2021

rshen91 commented Sep 7, 2021 •

edited

Loading

monfera Sep 9, 2021 •

edited

Loading

monfera Sep 9, 2021 •

edited

Loading

monfera left a comment •

edited

Loading

monfera left a comment •

edited

Loading