grafana / scenes

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

VizPanel: Allow configuring hover header offset #674

Closed Sergej-Vlasov closed 7 months ago

Sergej-Vlasov commented 7 months ago

Fixes #84830

Issue: When hoverHeader is enabled PanelMenu is being cropped. There are 2 issues:

Screenshot 2024-04-03 at 21 12 36



Solution:

Set hoverHeaderOffset prop on PanelChrome to prevent default offset of -32 in HoverWidget Definitely not ideal because it covers viz data but its quite tricky to portal HoverWidget and keep accessibility Open to comments on this one.

Screenshot 2024-04-03 at 21 10 30 Screenshot 2024-04-03 at 21 13 00
📦 Published PR as canary version: 4.5.5--canary.674.8612605644.0
:sparkles: Test out this PR locally via: ```bash npm install @grafana/scenes@4.5.5--canary.674.8612605644.0 # or yarn add @grafana/scenes@4.5.5--canary.674.8612605644.0 ```
CLAassistant commented 7 months ago

CLA assistant check
All committers have signed the CLA.

dprokop commented 7 months ago

Hm, in the old architecture the grid was rendered withing a scroll wrapper too, what's the difference then between the DOM of the old and new arch?

I don't think we should change the offset.

dprokop commented 7 months ago

Uhm, isn't that a matter of canvas-content not having top padding anymore?

Sergej-Vlasov commented 7 months ago

Uhm, isn't that a matter of canvas-content not having top padding anymore?

@dprokop Yes indeed, old arch had top padding of 16px and offset of -16px (to fill that padding). Currently we don't have padding on the canvas content and padding comes from controls above (which are sticky with high zIndex).

We could add top padding of 16px and offset of -16px to mimic old arch would look like that:

image

The only question is about how pretty this UX with that gap

dprokop commented 7 months ago

@Sergej-Vlasov noticed one more thing, in the old arch the offset is smaller for the panels at the top:

image

So there must be a collision detection or sth?

Sergej-Vlasov commented 7 months ago

@Sergej-Vlasov noticed one more thing, in the old arch the offset is smaller for the panels at the top:

image

So there must be a collision detection or sth?

Check is based on position so offset is -16 if panel at the top and -32 if anything else

const hoverHeaderOffset = (panel.gridPos?.y ?? 0) === 0 ? -16 : undefined;
Sergej-Vlasov commented 7 months ago

@dprokop Thinking of just passing through a hoverHeaderOffset prop in scenes and defining everything in main repo. Would this be the right place buildGridItemForPanel ?

torkelo commented 7 months ago

@Sergej-Vlasov fixed a related bug https://github.com/grafana/grafana/pull/85621

Not sure we can fix this only in buildGridItemForPanel without a behavior as you can move panels around and the offset would need to change

dprokop commented 7 months ago

Not sure we can fix this only in buildGridItemForPanel without a behavior as you can move panels around and the offset would need to change

Exactly. @Sergej-Vlasov we would need to add a behavior to DashboardGridItem that would observe the changes in y position and update VizPanel accordingly. But the VizPanel API would have to be updated to allow offset configuration and I personally don't like this. VizPanel should not care about any UI-specific props. It's the responsibility of the PanelChrome I think.

That's why maybe there's an easier solution that does not require us to observe grid position. The same problem may appear in layouts other than Grid and then the current method won't work. I think we may want not to rely on the y-position, but rather the constraints of the scroll container. We already use https://floating-ui.com/docs/usefloating to position Tooltips and I think that's what we possibly would may want to use to position the hover header. They have a middleware that guarantees visibility: https://floating-ui.com/docs/shift

Thoughts?

Sergej-Vlasov commented 7 months ago

@dprokop @torkelo Due to complications with portaling using FloatingUI I decided to proceed with temporary fix using hoverHeaderOffset and behaviour that updates it for the panels at the top only. So here I only pass in hoverHeaderOffset prop.

grafanabot commented 7 months ago

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