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

DashboardGrid: Attention state to handle keyboard and mouse focus #716

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

tskarhed
Copy link

@tskarhed tskarhed commented May 3, 2024

Addresses this issue grafana/grafana#47005 (comment)

Related PR in the Grafana repo: grafana/grafana#87317

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

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

@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

@tskarhed
Copy link
Author

tskarhed commented May 8, 2024

@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
Copy link
Author

tskarhed commented May 8, 2024

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
Copy link
Member

torkelo commented May 10, 2024

@tskarhed maybe GrafanaContext can have that?

@tskarhed
Copy link
Author

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

@torkelo
Copy link
Member

torkelo commented May 14, 2024

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

@tskarhed
Copy link
Author

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
Copy link
Member

torkelo commented May 16, 2024

@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
Copy link
Author

@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
Copy link
Author

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
Copy link
Author

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

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Sorry had this unpublished review comment from 1-2 days ago (done on mobile)

Will take a look more closely next week (traveling today)

Comment on lines 139 to 140
onFocus={() => panelAttentionService.setPanelWithAttention(model)}
onMouseMove={() => panelAttentionService.setPanelWithAttention(model)}
Copy link
Member

Choose a reason for hiding this comment

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

wonder if just emitting a new appEvent would be cleaner and simpler for this (no need to add a new runtime global service). The thing that feels hacky with these global runtime services is that their state cannot be subscribed to, but not a critical issue in this case.

Also a setPanelWithAttention in https://github.com/grafana/grafana/pull/87317/files#diff-51fff7911699e52be67bf25251d976007d8885bc55dcc1f8642464e52059f66fR10 takes HTML element but here your passing a scene model

Another thing, I think is the wrong focus level. The PanelChrome is the element that has tabIndex & keyboard focus (when you tab)

Copy link
Author

@tskarhed tskarhed May 24, 2024

Choose a reason for hiding this comment

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

I updated using panelIds instead now. I tend to agree with you. I think the approach you mention makes sense.

  • Add onFocus and onMouseMover handlers to PanelChrome
  • Emit an event wherever PanelChrome is used (no appEvents in grafana ui) Assume that the panelId exists in that context
  • Listen to the event in KeybindingSrv and set a property

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: 🔍 In review
Development

Successfully merging this pull request may close these issues.

None yet

2 participants