-
Notifications
You must be signed in to change notification settings - Fork 47
Customize tooltip demo #1075
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
base: main
Are you sure you want to change the base?
Customize tooltip demo #1075
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great and works as expected.
.setTitle('Panel 1') | ||
.setData(getQueryRunnerWithRandomWalkQuery()) | ||
//@ts-expect-error | ||
.setCustomFieldConfig('tooltipSeriesComponent', VizTooltipItemCustomComponent) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like a code smell to me @torkelo. I don't think we should use field config for such things given it's a serializable property of the dashboard. As much as I like this approach to customization, I think we should have an explicit, separate property for expressing "ui/runtime customizable" things. Like setCustomizations(customizationName, CustomComponent)
that's an API on panel plugins. Under the hood, I think it could be "merged" into fieldConfig (probably through a separate property) that's explicitly skipped when serializing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dprokop I am not really following, that seems a bit hackier / fragile to have global extension points, also not sure how it would work really. It need to be configured somehow via VizPanel / field config props (to control it per panel and field), and needs to be accessible inside grafana/ui (as we still have many viz components there), and grafana/runtime is not accessible there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think we should have an explicit, separate property for expressing "ui/runtime customizable" things. Like setCustomizations(customizationName, CustomComponent) that's an API on panel plugins.
How would an app plugin that wants to extend a VizPanel access the panel plugin API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mean how would the consumer of the panel configure / set the extension component, and configure options for it (per panel) in a way that it is local to the specific panel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also these custom components needs to be passed deep into visualizations and be configured per field (for example custom cell components), so feels very messy to have a parallel field config system
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What we could do is in PanelProps add a new property called customComponents with a dictionary to custom react components (key value), but untyped react components.
Then add this to VizPanel as well so it can pass it to panel. But then visualizations would need to pass along this dictionary deep into the rendering, and use to look up custom components (based on id/key referenced in field config).
But not 100% sure what we gain by this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@torkelo - agree with your points. I wasn't precise in my comment, when i said Like setCustomizations(customizationName, CustomComponent) that's an API on panel plugins.
.
fieldConfig
as data trasport for this configuration seems OK, it's the API that I think is not right. My suggestion would be to have:
VizPanelBuilder.setCustomizations(customization: 'tooltip' | ..., component: CustomizationComponent)
where the actual implementation would of course use this.setCustomFieldConfig(...)
under the hood. This way we can have
- Precise API definition what are the available viz customizations
- Type safety for customization component props if they would differ across customizations - i guess legend will be coming in the future as a customization point.
- No
@ts-expect-error
annotations like this in the plugins:
//@ts-expect-error
.setCustomFieldConfig('tooltipSeriesComponent', VizTooltipItemCustomComponent)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dprokop ah, I missunderstood you then. I thought you wanted to configure it via some plugin runtime hook (vs field config).
Adding strongly typed facades in the panel builders (that internally just do setCustomFieldConfig), seems like a good idea and easy to add.
@joannaWebDev what happened with this feature request? did it go anywhere? |
Main repo PR grafana/grafana#102341