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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

VizTooltips: Heatmap fixes and improvements #83876

Merged
merged 11 commits into from Mar 7, 2024
Merged

Conversation

leeoniya
Copy link
Contributor

@leeoniya leeoniya commented Mar 5, 2024

fixes:

this refactors code related to:

@leeoniya leeoniya added the no-changelog Skip including change in changelog/release notes label Mar 5, 2024
@leeoniya leeoniya self-assigned this Mar 5, 2024
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.0.x milestone Mar 5, 2024
@leeoniya leeoniya added no-backport Skip backport of PR area/tooltip labels Mar 5, 2024
@leeoniya leeoniya changed the title [WIP] VizTooltips: Heatmap fixes and improvements VizTooltips: Heatmap fixes and improvements Mar 6, 2024
@leeoniya leeoniya marked this pull request as ready for review March 6, 2024 14:42
@leeoniya leeoniya requested a review from a team as a code owner March 6, 2024 14:42
@leeoniya leeoniya requested review from nmarrs, drew08t and baldm0mma and removed request for a team and drew08t March 6, 2024 14:42
@@ -56,7 +56,7 @@ export const TimeSeriesTooltip = ({
seriesIdx,
mode,
sortOrder,
(field) => field.type === FieldType.number
(field) => field.type === FieldType.number || field.type === FieldType.enum
Copy link
Contributor Author

Choose a reason for hiding this comment

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

enum field fix

Comment on lines +589 to +592
focus: {
prox: 1e3,
dist: (u, seriesIdx) => (hRect?.sidx === seriesIdx ? 0 : Infinity),
},
Copy link
Contributor Author

Choose a reason for hiding this comment

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

heatmap cell hover prox fix

@@ -85,7 +85,7 @@ export const getContentItems = (
): VizTooltipItem[] => {
let rows: VizTooltipItem[] = [];

let allNumeric = false;
let allNumeric = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorting fix

@@ -772,6 +777,8 @@ export function heatmapPathsPoints(opts: PointsBuilderOpts, exemplarColor: strin
let fillPaths = [points];
let fillPalette = [exemplarColor ?? 'rgba(255,0,255,0.7)'];

let yShift = yLayout === HeatmapCellLayout.le ? -0.5 : yLayout === HeatmapCellLayout.ge ? 0.5 : 0;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this fixes vertical exemplar positioning in each cell. previously we assumed le (cell down from tick), but with ordinal it is centered, and can be manually set as well via panel opts.

Comment on lines -785 to -788
yVal -= 0.5; // center vertically in bucket (when tiles are le)
// y-randomize vertically to distribute exemplars in same bucket at same time
let randSign = Math.round(Math.random()) * 2 - 1;
yVal += randSign * 0.5 * Math.random();
Copy link
Contributor Author

@leeoniya leeoniya Mar 6, 2024

Choose a reason for hiding this comment

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

this removes exemplar y position randomization within each cell and provides stability, but also maybe overlap. probably a better trade-off. we can look at clustering or multi-hover in the future.

Copy link
Contributor

@baldm0mma baldm0mma left a comment

Choose a reason for hiding this comment

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

  • timeseries tooltip was not showing enum type fields
  • fixes numeric tooltip sorting (for real this time)
  • heatmap tooltip was only activating within 30px proximity to y-center of each cell
  • heatmap data links code was slow and did not support field labels
  • [-] tooltips work as expected when newVizTooltips FF is toggled on and off - for this I did notice one thing: when hovering a value with the old tooltips, the color scale at the bottom would display the hovered value on the range, but with the new tooltips, this functionality seems to be missing. Is this expected?
Screenshot 2024-03-06 at 2 39 25鈥疨M Screenshot 2024-03-06 at 2 38 42鈥疨M

@leeoniya
Copy link
Contributor Author

leeoniya commented Mar 7, 2024

tooltips work as expected when newVizTooltips FF is toggled on and off - for this I did notice one thing: when hovering a value with the old tooltips, the color scale at the bottom would display the hovered value on the range, but with the new tooltips, this functionality seems to be missing. Is this expected?

yes, this is expected for now. this was the only panel with some kind of live legend-tooltip behavior, so was too much of a snowflake to keep in right now. we will revisit this concept holistically for all panels as part of the legends work.

the live color scale was moved into the tooltip as a toggle-able option in panel edit (same as live histogram in tooltip).

@leeoniya leeoniya merged commit d549a3a into main Mar 7, 2024
18 checks passed
@leeoniya leeoniya deleted the leeoniya/viztooltips-fixes-2 branch March 7, 2024 01:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/frontend area/panel/heatmap area/panel/timeseries The main time series Graph panel area/tooltip no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants