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

VizPanel: Allow configuring hover header offset #674

Merged
merged 4 commits into from
Apr 9, 2024

Conversation

Sergej-Vlasov
Copy link
Contributor

@Sergej-Vlasov Sergej-Vlasov commented Apr 3, 2024

Fixes #84830

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

  • crop on the first panel due to hidden overflow of scroll wrapper
Screenshot 2024-04-03 at 21 10 10

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

✨ Test out this PR locally via:

npm install @grafana/scenes@4.5.5--canary.674.8612605644.0
# or 
yarn add @grafana/scenes@4.5.5--canary.674.8612605644.0

@CLAassistant
Copy link

CLAassistant commented Apr 3, 2024

CLA assistant check
All committers have signed the CLA.

@dprokop
Copy link
Member

dprokop commented Apr 4, 2024

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

dprokop commented Apr 4, 2024

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

@Sergej-Vlasov
Copy link
Contributor Author

Sergej-Vlasov commented Apr 4, 2024

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

dprokop commented Apr 4, 2024

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

Sergej-Vlasov commented Apr 4, 2024

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

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

torkelo commented Apr 5, 2024

@Sergej-Vlasov fixed a related bug grafana/grafana#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
Copy link
Member

dprokop commented Apr 5, 2024

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?

@mdvictor mdvictor added patch Increment the patch version when merged release Create a release when this pr is merged labels Apr 9, 2024
@Sergej-Vlasov
Copy link
Contributor Author

@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.

@Sergej-Vlasov Sergej-Vlasov marked this pull request as ready for review April 9, 2024 08:41
@dprokop dprokop added release Create a release when this pr is merged and removed release Create a release when this pr is merged labels Apr 9, 2024
@dprokop dprokop changed the title Prevent panel hover header crop VizPanel: Allow configuring hover header offset Apr 9, 2024
Copy link
Contributor

@ivanortegaalba ivanortegaalba left a comment

Choose a reason for hiding this comment

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

As we agreed, let's go for this one 👍

@Sergej-Vlasov Sergej-Vlasov merged commit a6e3300 into main Apr 9, 2024
3 checks passed
@Sergej-Vlasov Sergej-Vlasov deleted the serge/prevent-panel-hover-header-crop branch April 9, 2024 15:03
@grafanabot
Copy link
Contributor

🚀 PR was released in v4.5.6 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Increment the patch version when merged release Create a release when this pr is merged released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Panel menu not visible when panel has no title
7 participants