fix(heatmap): filter out tooltip picked shapes in x-axis area #1351

rshen91 · 2021-09-01T21:00:07Z

Summary

This bug fix removes the tooltip when hovering over the x-axis.

Before

before.mp4

After

after.mp4

Details

Adjust cursor logic

Issues

Closes #1215

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)

markov00

Logically looks correct, do you think we can apply this pickedShapes filter directly into the getPickedShapes selector to control and fix that in a single place?

rshen91 · 2021-09-02T18:58:18Z

Logically looks correct, do you think we can apply this pickedShapes filter directly into the getPickedShapes selector to control and fix that in a single place?

I'm definitely going to try -- the issue I'm facing is the return type of pickedShapes can be an array of Cells or TextBox so it's more complicated than I had hoped :)

rshen91 · 2021-09-02T20:44:39Z

@markov00 in 8358dd8 I cleaned it up in the picked_shapes selector but I was struggling to find a cleaner way to deal with the cursor logic in get_cursor_pointer.ts (like a way to avoid the length check in get_cursor_pointer). I was trying to move that length check into picked_shapes but kept facing issues with typescript and returning boolean values. Is there a cleaner way I'm overlooking or is the refactoring in 8358dd8 ok? Thanks

rshen91 · 2021-09-02T21:49:13Z

jenkins test this

markov00 · 2021-09-03T09:32:35Z

packages/charts/src/chart_types/heatmap/state/selectors/picked_shapes.ts

+    const arrayPicker = picker(x, y) as Cell[];
+    return Array.isArray(picker(x, y))
+      ? arrayPicker.filter(({ y }) => y < gridParams.gridCellHeight * gridParams.pageSize)
+      : picker(x, y);


My suggestion is:

first of all call the picker function only one time

don't cast it to Cell[] because it is not required in the code

the ternary check isArray already allows TS and also JS to define if the pickedData is an array of Cell or a TextBox

Suggested change

const arrayPicker = picker(x, y) as Cell[];

return Array.isArray(picker(x, y))

? arrayPicker.filter(({ y }) => y < gridParams.gridCellHeight * gridParams.pageSize)

: picker(x, y);

const pickedData = picker(x, y);

return Array.isArray(pickedData)

? pickedData.filter(({ y }) => y < gridParams.gridCellHeight * gridParams.pageSize)

: pickedData;

Thank you! 6785db6 for changes and added the story changes into this commit as well

markov00 · 2021-09-03T09:35:29Z

Can you also please update the story with a knob that remove the filter on the BABY_NAME data so reviewers can test this easily without touching the code? thanks

markov00

LGTM, good to merge after fixing the highlighted code change

markov00 · 2021-09-03T16:36:11Z

storybook/stories/heatmap/2_categorical.story.tsx

+  const filterData = boolean('filter BABYNAME_DATA', false);
+  const data = filterData ? BABYNAME_DATA : BABYNAME_DATA.filter(([year]) => year > 1950 && year < 1960);


Isn't that the inverse?
If I check the filter knob to enable the filter I should see only a partial set.
If the filter check is off, then I should see all the data available.
Two code change:

reverse the ternary operation and change the default value of the checkbox to true

rename the filter to: filter dataset. The BABYNAME_DATA is something we don't want to show to the user

Yikes good catch thanks 38147b4

# [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:58:06Z

🎉 This PR is included in version 35.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

fix: filter out tooltip picked shapes into x axis

0a5f8bf

rshen91 marked this pull request as draft September 1, 2021 21:00

rshen91 changed the title ~~fix(heatmap): filter out tooltip picked shapes into x-axis~~ fix(heatmap): filter out tooltip picked shapes in x-axis area Sep 1, 2021

rshen91 added bug Something isn't working :heatmap Heatmap/Swimlane chart related issue labels Sep 1, 2021

fix: update cursor pointer

1fc046a

markov00 reviewed Sep 2, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into heatmap-page-tooltip

c4fb452

refactor: clean up picked shapes selector

8358dd8

rshen91 requested a review from markov00 September 2, 2021 21:35

markov00 reviewed Sep 3, 2021

View reviewed changes

fix: code review changes

6785db6

rshen91 marked this pull request as ready for review September 3, 2021 13:04

rshen91 requested a review from markov00 September 3, 2021 13:26

markov00 approved these changes Sep 3, 2021

View reviewed changes

fix: fix ternary in story

38147b4

rshen91 merged commit 174047d into elastic:master Sep 3, 2021

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): filter out tooltip picked shapes in x-axis area #1351

fix(heatmap): filter out tooltip picked shapes in x-axis area #1351

rshen91 commented Sep 1, 2021 •

edited

Loading

markov00 left a comment

rshen91 commented Sep 2, 2021

rshen91 commented Sep 2, 2021

rshen91 commented Sep 2, 2021

markov00 Sep 3, 2021

rshen91 Sep 3, 2021

markov00 commented Sep 3, 2021

markov00 left a comment

markov00 Sep 3, 2021

rshen91 Sep 3, 2021

nickofthyme commented Sep 13, 2021

		const filterData = boolean('filter BABYNAME_DATA', false);
		const data = filterData ? BABYNAME_DATA : BABYNAME_DATA.filter(([year]) => year > 1950 && year < 1960);

fix(heatmap): filter out tooltip picked shapes in x-axis area #1351

fix(heatmap): filter out tooltip picked shapes in x-axis area #1351

Conversation

rshen91 commented Sep 1, 2021 • edited Loading

Summary

Before

After

Details

Issues

Checklist

markov00 left a comment

Choose a reason for hiding this comment

rshen91 commented Sep 2, 2021

rshen91 commented Sep 2, 2021

rshen91 commented Sep 2, 2021

markov00 Sep 3, 2021

Choose a reason for hiding this comment

rshen91 Sep 3, 2021

Choose a reason for hiding this comment

markov00 commented Sep 3, 2021

markov00 left a comment

Choose a reason for hiding this comment

markov00 Sep 3, 2021

Choose a reason for hiding this comment

rshen91 Sep 3, 2021

Choose a reason for hiding this comment

nickofthyme commented Sep 13, 2021

rshen91 commented Sep 1, 2021 •

edited

Loading