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

VizLegend: Represent line style in series legend and tooltip #87558

Merged
merged 8 commits into from May 10, 2024

Conversation

domasx2
Copy link
Contributor

@domasx2 domasx2 commented May 9, 2024

What is this feature?

This PR makes panel legend & viz tooltip color indicator also represent line style of a series: solid, dotted or dashed. Only works if gradient is not set, because we can't represent it in dashed/ dotted form with current appraoch.

dot-dash-cmp
dot-dash-cmp

Why do we need this feature?

This is especially useful with time comparison feature where UX calls for comparison series to have same color but different line style. Eg, app o11y uses dashed lines for previous period series. Without legend representing line style it is harder to distinguish legend items.

Who is this feature for?

Dashboard users, plugins.

Special notes for your reviewer:

I went ahead and made viz tooltip reuse SeriesIcon component for color indicator

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@domasx2 domasx2 added area/frontend add to changelog no-backport Skip backport of PR internal for issues made by grafanistas area/dataviz Anything that relates to Data Visualisation work but is not specific to one panel labels May 9, 2024
@domasx2 domasx2 requested a review from a team as a code owner May 9, 2024 13:22
@domasx2 domasx2 requested review from codeincarnate, baldm0mma and leeoniya and removed request for a team May 9, 2024 13:22
@grafana-delivery-bot grafana-delivery-bot bot added this to the 11.1.x milestone May 9, 2024
@leeoniya
Copy link
Contributor

leeoniya commented May 9, 2024

would it be possible to avoid extra DOM here? especially for tooltip that has to re-render up to 60 times per sec, this will add a lot of overhead for something that we might be able to do with a single React.memo'd SVG element or clever CSS.

@domasx2
Copy link
Contributor Author

domasx2 commented May 9, 2024

would it be possible to avoid extra DOM here? especially for tooltip that has to re-render up to 60 times per sec, this will add a lot of overhead for something that we might be able to do with a single React.memo'd SVG element or clever CSS.

I'll try. What's the case where dom is recreated 60 times per second though? :-O would React.memo'ing the divs work too?

@leeoniya
Copy link
Contributor

leeoniya commented May 9, 2024

I'll try. What's the case where dom is recreated 60 times per second though?

sorry, i usually mean react's v-tree, which very unfree to re-create on ever mousemove. imagine you have 30 series in a tooltip. and dashboard shared tooltip enabled. this adds up very fast.

@domasx2
Copy link
Contributor Author

domasx2 commented May 9, 2024

I removed extra divs and refactored with border styling, which gives almost the same result in Chrome
image

Will test with more browsers tomorrow

@leeoniya
Copy link
Contributor

leeoniya commented May 9, 2024

I removed extra divs and refactored with border styling, which gives almost the same result in Chrome

thanks :)

@domasx2 domasx2 requested review from a team as code owners May 10, 2024 07:44
@domasx2
Copy link
Contributor Author

domasx2 commented May 10, 2024

Problem with border based solution is that it's not consistent across render engines. In firefox:
image

@domasx2
Copy link
Contributor Author

domasx2 commented May 10, 2024

So border did not work out, too inconsistent across borwsers.
Refactored to third attempt which seems to work well: repeated background images using radial-gradient and linear-gradient. This gives consistent result across all browsers.
Caveat is that it doesn't render gradient too, so in case gradient is set it will default to previous looks

@domasx2 domasx2 requested a review from leeoniya May 10, 2024 09:12
Copy link
Contributor

@leeoniya leeoniya left a comment

Choose a reason for hiding this comment

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

awesome! ❤️ :shipit:

@domasx2 domasx2 merged commit 708bcda into main May 10, 2024
29 checks passed
@domasx2 domasx2 deleted the domas-line-style-legend branch May 10, 2024 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/dataviz Anything that relates to Data Visualisation work but is not specific to one panel area/frontend internal for issues made by grafanistas no-backport Skip backport of PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants