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: Hide dimension configuration when tooltip mode is hidden #81627

Merged
merged 3 commits into from
Feb 14, 2024

Conversation

codeincarnate
Copy link
Collaborator

What is this feature?

This PR hides the width and height settings for the tooltip when the mode is set to Hidden as these options won't apply in that case.

This can be seen in action below:

Screen.Recording.2024-01-31.at.5.30.45.PM.mov

Why do we need this feature?

This removes potential confusion when configuring tooltips as in this case the width and height settings will not have any effect.

Who is this feature for?

Users that want to disable tooltips.

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.

@codeincarnate codeincarnate added this to the 10.4.x milestone Jan 31, 2024
@codeincarnate codeincarnate requested a review from a team January 31, 2024 10:33
@codeincarnate codeincarnate requested review from a team as code owners January 31, 2024 10:33
@codeincarnate codeincarnate requested review from ashharrison90, JoaoSilvaGrafana and sunker and removed request for a team January 31, 2024 10:33
@grafana-pr-automation grafana-pr-automation bot requested review from a team and removed request for a team January 31, 2024 10:38
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.

Looks like this broke a test. But after that's fixed, LGTM.

@@ -52,6 +52,7 @@ export function addTooltipOptions<T extends OptionsWithTooltip>(
integer: true,
placeholder: '300',
},
showIf: (options: T) => options.tooltip?.mode !== TooltipDisplayMode.None,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
showIf: (options: T) => options.tooltip?.mode !== TooltipDisplayMode.None,
showIf: (options: T) => config.featureToggles.newVizTooltips && options.tooltip?.mode !== TooltipDisplayMode.None,

Copy link
Contributor

Choose a reason for hiding this comment

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

hm just tried to add this locally and ran into issue where it seems we are not able to pull the config object from grafana runtime anymore in the grafana-ui package... made this thread in frontend channel to ask about this 😬

Copy link
Contributor

Choose a reason for hiding this comment

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

ugh, yeah, I forgot about imports 😬 it's not possible as far as I know, I've also asked a while ago and we ended up moving Timeseries out of grafana-ui and do the imports.. So maybe we should leave it as is, we're going to remove the feature flag in the near future anyway 😞

Copy link
Contributor

Choose a reason for hiding this comment

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

Following up from thread it appears there is no way to access the feature toggles from the run time config

The only way that this would be feasible is through passing this info as a prop to the component - which could be a viable solution for the time being

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We may need to talk about this as currently these options are being exposed regardless of the tooltip usage. This PR doesn't impact that so I may go ahead and merge anyhow, and we can address this being displayed in another PR

@adela-almasan
Copy link
Contributor

Hey, looks good! I just realized that these options are just for the new tooltips, so I suggested to add that to the condition. Also Heatmap needs to be updated and it should be good to go 😁

@grafana-pr-automation grafana-pr-automation bot requested review from a team and removed request for a team January 31, 2024 22:45
@grafana-pr-automation grafana-pr-automation bot requested review from a team and removed request for a team January 31, 2024 22:50
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.

LGTM - we just need to figure out if we can access feature toggle from runtime config in grafana-ui 😬

@@ -52,6 +52,7 @@ export function addTooltipOptions<T extends OptionsWithTooltip>(
integer: true,
placeholder: '300',
},
showIf: (options: T) => options.tooltip?.mode !== TooltipDisplayMode.None,
Copy link
Contributor

Choose a reason for hiding this comment

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

hm just tried to add this locally and ran into issue where it seems we are not able to pull the config object from grafana runtime anymore in the grafana-ui package... made this thread in frontend channel to ask about this 😬

@codeincarnate
Copy link
Collaborator Author

Going to merge this for now as these fields are already showing regardless. Going to make sure to do a follow-up for this one so we can hopefully disable these fields showing when using old tooltips.

@codeincarnate codeincarnate changed the title Tooltips: Hide dimensions when tooltip mode is hidden Tooltips: Hide dimension configuration when tooltip mode is hidden Feb 14, 2024
@codeincarnate codeincarnate merged commit 26b25da into main Feb 14, 2024
24 checks passed
@codeincarnate codeincarnate deleted the tooltip-hide-dims branch February 14, 2024 16:35
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants