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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

VizTooltips: Render data links only when anchored #83737

Merged
merged 1 commit into from
Feb 29, 2024

Conversation

leeoniya
Copy link
Contributor

@leeoniya leeoniya commented Feb 29, 2024

turns out it's pretty expensive to re-generate datalinks while live-hovering dense datasets.

this rolls back the changes in #83638 until we can improve the datalink gen performance.

(also removes an unnecessary <div> wrapper)

@leeoniya leeoniya self-assigned this Feb 29, 2024
@leeoniya leeoniya requested a review from a team as a code owner February 29, 2024 22:27
@leeoniya leeoniya requested review from baldm0mma and drew08t and removed request for a team February 29, 2024 22:27
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.0.x milestone Feb 29, 2024
Copy link
Contributor

@nmarrs nmarrs left a comment

Choose a reason for hiding this comment

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

Code LGTM - only potential concern I have with this is potential inconsistencies with when data links appear in tooltips across visualizations (unless our plan is to implement this optimization across all visualizations).

Another thing to consider is discoverability of datalinks (i.e. user might not be aware they are even an option and so might miss them entirely unless they pin the tooltip) - whereas before it was a visible indicator / reason to pin tooltip in the first place - maybe we can have some indication / text letting user know that datalinks exist or something 🤷

Screen.Recording.2024-02-29.at.11.35.29.PM.mov

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.

LGTM!

@baldm0mma
Copy link
Contributor

baldm0mma commented Feb 29, 2024

Code LGTM - only potential concern I have with this is potential inconsistencies with when data links appear in tooltips across visualizations (unless our plan is to implement this optimization across all visualizations).

Another thing to consider is discoverability of datalinks (i.e. user might not be aware they are even an option and so might miss them entirely unless they pin the tooltip) - whereas before it was a visible indicator / reason to pin tooltip in the first place - maybe we can have some indication / text letting user know that datalinks exist or something 🤷

Screen.Recording.2024-02-29.at.11.35.29.PM.mov

@nmarrs After looking at #83638, it looks like rendering the datalinks only if the tooltip is pinned was already the de facto behavior, so hopefully the discoverability piece won't be any worse? Maybe I'm reading it wrong?

@baldm0mma baldm0mma added no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes labels Feb 29, 2024
@leeoniya
Copy link
Contributor Author

leeoniya commented Feb 29, 2024

Another thing to consider is discoverability of datalinks (i.e. user might not be aware they are even an option and so might miss them entirely unless they pin the tooltip) - whereas before it was a visible indicator / reason to pin tooltip in the first place - maybe we can have some indication / text letting user know that datalinks exist or something 🤷

totally agree on discoverability; that was the main reason for making them visible unconditionally in the previous PR. but the old tooltips only showed them when anchored as well, so we're not regressing anything. i dont intend for this to be the endgame, but optimizing getLinks() touches aspects of fieldOverrides, scopedVars, url parsing, etc. -- things we definitely dont want to poke right now.

one idea is maybe to defer link rendering after a user has paused over a datapoint for 100ms. this way it's both discoverable and fast when mousing around without intent, but that requires additional debounce changes to the tooltip plugin. let's just do the easy thing for now :)

@leeoniya
Copy link
Contributor Author

leeoniya commented Feb 29, 2024

only potential concern I have with this is potential inconsistencies with when data links appear in tooltips across visualizations (unless our plan is to implement this optimization across all visualizations).

yeah. heatmap tooltip is a bit of its own beast currently and has custom link re-gen code in the tooltip itself due to dataframe structure (you know ;). it needs to be refactored to share similar logic with the others, but i'm gonna leave this to another PR. added it to this epic: #83632

@leeoniya leeoniya modified the milestone: 11.0.x Feb 29, 2024
@leeoniya leeoniya merged commit 859ecf2 into main Feb 29, 2024
39 checks passed
@leeoniya leeoniya deleted the leeoniya/viztooltips-pinned-links branch February 29, 2024 23:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants