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

Tooltips: Support long labels #77735

Merged
merged 10 commits into from
Nov 16, 2023
Merged

Tooltips: Support long labels #77735

merged 10 commits into from
Nov 16, 2023

Conversation

Develer
Copy link
Contributor

@Develer Develer commented Nov 6, 2023

What is this feature?

This PR provide tooltip's long labels support.

Which issue(s) does this PR fix?:

Fixes #77247

Special notes for your reviewer:

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.

@Develer Develer added area/frontend add to changelog area/tooltip no-backport Skip backport of PR no-changelog Skip including change in changelog/release notes area/dataviz Anything that relates to Data Visualisation work but is not specific to one panel labels Nov 6, 2023
@Develer Develer added this to the 10.3.x milestone Nov 6, 2023
@Develer Develer self-assigned this Nov 6, 2023
@Develer Develer requested a review from a team as a code owner November 6, 2023 16:03
@Develer Develer requested review from leeoniya and adela-almasan and removed request for a team November 6, 2023 16:03
@adela-almasan
Copy link
Contributor

Hey, good job on this one! 🎉 While testing I noticed some things that were not covered:

  1. Long labels in series list (used in Trend), also needs an overflow hidden + ellipsis
  2. In header, if we have a label that's too long, the value is pushed too much to the right and it overlaps with the close button
  3. In header, if we have a value that's too long, it overlaps the label

heatmap_long_value
trend_long_series_label

@adela-almasan
Copy link
Contributor

Feedback:
cc @lukasztyrala

  1. In some cases the header value gets completely hidden
    hidden value

  2. The changes we made for header need to be applied for all the components we use in tooltips
    overflow

  3. The labels/values seem to be missing the hover functionality when pinned (add tooltip with full value)

@Develer
Copy link
Contributor Author

Develer commented Nov 13, 2023

@adela-almasan This #2 point happened because it is a custom component. In trend tooltip case it's <SeriesList />.

@adela-almasan
Copy link
Contributor

@Develer SeriesList is one of the tooltips components - currently used in Trend, but will also be used in Timeseries and Candlestick and possibly other places, so we need to cover that one too. We need to apply the changes everywhere we've using labels/values.

@Develer
Copy link
Contributor Author

Develer commented Nov 13, 2023

@adela-almasan got it, then I'll update that one also 👍🏻

@lukasztyrala
Copy link
Member

Let's acknowledge that we are dealing with edge cases here, and rarely will there be a situation where both labels and values are super long. But it can happen. A few things to consider:

  1. We should provide reasonable defaults for the things displayed in the header. F.e. for the trend panel, we should render the x label and value.

Trend

  1. We should consider the label component as an interactive and dynamic element that can adapt to options (f.e. where the ellipsis should happen, do people want to wrap the labels, etc.)

  2. The tooltip should be responsive.

2 and 3 from above are a song of future improvements, so for now, we need to introduce some rules for the logic:

  1. Displaying the value should always be a priority.

Long list (anchored)

  1. There should be a minimum label width (at least three characters displayed), and the ellipsis would be applied to values only in a case like this.

Long list (anchored)_extremly

If 4 and 5 are hard to calculate or implement, than we should go back to responsiveness? :-)

@@ -18,13 +14,18 @@ export const SeriesList = ({ series }: SeriesListProps) => {
return (
<>
{series.map((series, index) => {
// eslint-disable-next-line @typescript-eslint/consistent-type-assertions
const label = series.label as string;
Copy link
Contributor Author

@Develer Develer Nov 15, 2023

Choose a reason for hiding this comment

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

Hi all! I have types clashing. The question is about the Trend tooltip. Do we really need SeriesList component? It looks totally replaceable by VizTooltipRow component I implemented here.
I noticed that in SeriesList the label can be label?: React.ReactNode type. Are there any scenarios we have to render ReactNode in a label instead of just a string?
Can we take a look at this part together?

cc @adela-almasan

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey! Context PR for the React.ReactNode type - probably we'll get the chance to test more with complex cases with timeseries. I think SeriesList looks like a wrapper for VizTooltipRow now, so that should be fine - we can iterate depending on how things evolve.

@adela-almasan
Copy link
Contributor

@Develer I'm ok approving and merging if the approach (value on second row) is the one you and @lukasztyrala have discussed yesterday. I can't say it's my favorite, but we can obviously iterate.

Screenshot 2023-11-15 at 11 42 00 AM

Screenshot 2023-11-15 at 11 47 23 AM

@Develer Develer merged commit 6f0c539 into main Nov 16, 2023
18 checks passed
@Develer Develer deleted the 77247-tooltips-long-labels branch November 16, 2023 13:53
@aangelisc aangelisc modified the milestones: 10.3.x, 10.2.3 Dec 21, 2023
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 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.

Tooltip: Support long labels
4 participants