grafana / scenes

Build Grafana dashboards directly in your Grafana app plugins.
https://grafana.com/developers/scenes
Apache License 2.0
133 stars 20 forks source link

VizPanelRenderer: Emit SetPanelAttention event #716

Closed tskarhed closed 4 months ago

tskarhed commented 4 months ago

Addresses this issue https://github.com/grafana/grafana/issues/47005#issuecomment-2089912829

Related PR in the Grafana repo: https://github.com/grafana/grafana/pull/87317

Is this a good place to add this? Also, are there some drawbacks that may cause performance issues?

📦 Published PR as canary version: 4.24.4--canary.716.9300035734.0
:sparkles: Test out this PR locally via: ```bash npm install @grafana/scenes@4.24.4--canary.716.9300035734.0 # or yarn add @grafana/scenes@4.24.4--canary.716.9300035734.0 ```
tskarhed commented 4 months ago

@tskarhed is it not better to have this logic in PanelChrome? Scenes support many layout options (not only GridLayout), would be nice to share the logic between them

True, it may be a bit more brittle though, causing multiple panels to have attention. But I can give it a go.

tskarhed commented 4 months ago

Ah, yes, that was the reason. If you have one panel in focus and then move the mouse over another, they both have attention, so you need something above keeping track of the attention state.

torkelo commented 4 months ago

@tskarhed maybe GrafanaContext can have that?

tskarhed commented 4 months ago

@torkelo If used with PanelChrome, wouldn't that be a dependency on core? And PanelChrome no knowledge of panel id :/

torkelo commented 4 months ago

@tskarhed grid / items does not know panel id either , maybe VizPanelRenderer can set attention? Just a question where state should live

tskarhed commented 4 months ago

Okay, for non-scenes I have it working with the state living in KeyboardSrv (which is accessible with the useGrafana hook). The the update still happens in the GridItem part (haven't figured out where else this would work), but it could easily be added to any component in core and be extended.

For Scenes, it seems like VizPanelRenderer is a good place. It can emit an event when it gets attention (preferably it would also be able to read the current attention state to determine if it has attention in order to avoid a ton of events being emitted on MouseMove). Just not sure how to read this possible parent state. At the moment I'm looking at storing the state in DashboardScene, and have it subscribe to the event. The nice thing about the scenes approach is that it is easy to pass around the ScenesObject instead of ids.

torkelo commented 4 months ago

@tskarhed why not use useGrafana and the KeyboardSrv to set state for scenes as well inside VizPanelRenderer?

Ah, plugins platform has not yet made useGrafana accessible to plugins :(

Then I don't know, maybe we need a new context that plugins (ie scenes) can interact with?

tskarhed commented 4 months ago

@torkelo Good point. At the moment there is also duplicate functionality for the keybindings, which I could consolidate. setupKeyboardShortcuts for Scenes and KeybingingSrv for core. These could probably be handled the same way (although withFocusedPanel passing the actual Scene object is super useful, compared to just the panel id).

Maybe I'll look into exposing either useGrafana or just the keybindings if there may be unintended consequences.

tskarhed commented 4 months ago

I created a service this time around. There are still some typing issues with VizPanel, and onMouseMove should probably be debounced, or something of the sort.

tskarhed commented 4 months ago

@torkelo I have updated the approach using a service instead. Please tell me what you think.

tskarhed commented 4 months ago

It still has the @ts-ignore comments (which are needed at the moment), but it works using the canary grafana/ui

grafanabot commented 4 months ago

:rocket: PR was released in v4.24.4 :rocket: