-
Notifications
You must be signed in to change notification settings - Fork 123
fix(heatmap): update tooltip visibility logic to handle empty tooltip info #2661
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
Conversation
buildkite update vrt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked and the tooltip has already a property called isVisible
. For the specific case we are covering, we are setting it to true
:
elastic-charts/packages/charts/src/chart_types/heatmap/state/selectors/tooltip.ts
Lines 87 to 100 in 13d9455
tooltipInfo.values.push({ | |
label: '', | |
color: Colors.Transparent.keyword, | |
isHighlighted: false, | |
isVisible: true, | |
seriesIdentifier: { | |
specId: spec.id, | |
key: spec.id, | |
}, | |
value: `${pickedShapes.value}`, | |
formattedValue: `${pickedShapes.text}`, | |
datum: pickedShapes.value, | |
displayOnly: true, | |
}); |
Have you considered updating this value and using it in isTooltipVisibleSelector
? For example:
isVisible: Boolean(pickedShapes.text)
And then in the selector:
visible: tooltipInfo.values.some(({ isVisible }) => isVisible) || pinned,
Otherwise, you can try to add a condition like the following at the beginning of isTooltipVisibleSelector
:
if (
type === TooltipType.None ||
tooltipInfo.values.length === 0 ||
(tooltipInfo.values.length === 1 && !tooltipInfo.values[0]?.formattedValue)
) {
return {
visible: false,
isExternal: false,
displayOnly: false,
isPinnable: false,
};
}
but this depends on how you build the tooltip in packages/charts/src/chart_types/heatmap/state/selectors/tooltip.ts
Nice suggestion @mariairiartef! I used the |
buildkite test this |
buildkite update vrt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation wise it LGTM. I've doubts if we need to apply the same logic also to the Y axis value
and X axis value
when the mouse pointer is on the chart and one of the axis has an empty value (see second screenshot that shows an empty Y Value) but I believe we can discuss this offline and and add it in case in a subsequent phase.
Summary
The heatmap tooltip is no longer visible if it's empty.
Before the fix:

After the fix:

fix #2546
Details
Issues
Checklist
:xy
,:partition
):interactions
,:axis
)closes #123
,fixes #123
)packages/charts/src/index.ts
light
anddark
themes