fix(heatmap): filter out tooltip picked shapes in x-axis area#1351
fix(heatmap): filter out tooltip picked shapes in x-axis area#1351rshen91 merged 6 commits intoelastic:masterfrom
Conversation
markov00
left a comment
There was a problem hiding this comment.
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 :) |
|
@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 |
|
jenkins test this |
| const arrayPicker = picker(x, y) as Cell[]; | ||
| return Array.isArray(picker(x, y)) | ||
| ? arrayPicker.filter(({ y }) => y < gridParams.gridCellHeight * gridParams.pageSize) | ||
| : picker(x, y); |
There was a problem hiding this comment.
My suggestion is:
- first of all call the
pickerfunction only one time - don't cast it to
Cell[]because it is not required in the code - the ternary check
isArrayalready allows TS and also JS to define if the pickedData is an array of Cell or a TextBox
| 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; |
There was a problem hiding this comment.
Thank you! 6785db6 for changes and added the story changes into this commit as well
|
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
left a comment
There was a problem hiding this comment.
LGTM, good to merge after fixing the highlighted code change
| const filterData = boolean('filter BABYNAME_DATA', false); | ||
| const data = filterData ? BABYNAME_DATA : BABYNAME_DATA.filter(([year]) => year > 1950 && year < 1960); |
There was a problem hiding this comment.
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
# [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.
|
🎉 This PR is included in version 35.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Summary
This bug fix removes the tooltip when hovering over the x-axis.
Before
before.mp4
After
after.mp4
Details
Issues
Closes #1215
Checklist
:xy,:partition)closes #123,fixes #123)