Autofit: Fit widget in the screen depending on the height #658
Conversation
| public getUrlState() { | ||
| return { | ||
| autofitpanels: this.state.autoFit ? '' : undefined, | ||
| }; | ||
| } | ||
|
|
||
| public updateFromUrl(values: SceneObjectUrlValues) { | ||
| const autoFit = values.autofitpanels; | ||
|
|
||
| if (typeof autoFit === 'string') { | ||
| this.setState({ | ||
| autoFit: true, | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
This kind of feels wrong to have the autofit as a part of the core layour implementation. I think we should solve this as an addon to the SceneGridLayout. Maybe we could have a "hook" that one could pass to the ScenegGidLayout via state (sth like processGridLayout) that would be called from buildGridLayout when you call cells = fitPanelsInHeight(cells, height);.
Then that code would become:
if (this.state.processGridLayout) {
cells = processGridLayout(cells, height);
}
And then the Autofit would be a behavior implemented in Grafana core that would provide that layout processing implementation and URL sync. WDYT @ivanortegaalba ?
There was a problem hiding this comment.
Yep, that would work too. But I think it would be useful for a non-dashboard app that can be displayed in "TV Mode" or on a screen. So, having an option that says "fitContent" or "fitVertical" as a boolean would be easier since the size of the widget, the margins, the row height and everything else is managed by the core layout. Otherwise we need to export all those measures, and make them configurable, in order to properly configure the layout.
So, IMO, the simplest solution is to keep the manipulation at the core where all the params are set.
There was a problem hiding this comment.
Yes and no IMO - the manipulation is grid units only, building layout does not require any knowledge about measures like row height or gap.
I see your point, tho I have not seen any asks from app plugin devs to add such an API. Not sure how realistic it is for the apps to be used in the TV mode, given they are way more interactive that a dashboard.
Is there any way we can at least make the URL sync a core specific behavior? It feels very wrong to grids auto fit behavior to URL by default.
There was a problem hiding this comment.
We could also deprecate this legacy experimental feature and implement it more properly (with flex layout or some other more dynamic layout).
But agree I do not think the core scenes lib needs to handle it
There was a problem hiding this comment.
I am happy to mark it as UNSAFE_fitPanels to not invite you to use it, as we did with UNSAFE_nowDelay.
But at least we need to port it somehow, don't we?
There was a problem hiding this comment.
But I think it would be useful for a non-dashboard app that can be displayed in "TV Mode" or on a screen.
apps do not use the absolute grid layout
There was a problem hiding this comment.
I think all use FlexLayout or CSSGridLayout which can easily be made "autofit" with simple CSS
There was a problem hiding this comment.
but looking closer at the implementation, it is really nice place for the logic to sit in between the runtime grid model and the model we pass to the react-grid-layout component
so maybe an unsafe prop is ok
|
Just coming in here to ask if this issue is still present in the new implementation grafana/grafana#79313 |
This issue is caused by not taking into account the height taken by the variables toolbar. This won't happen with this new approach, since the viewport is calculated as the size available on the screen, not fixing any height for the navbar or any other item out of the dashboard element. The old architecture, the calculation includes fixed values like the navbar. |
ivanortegaalba
added
usecase/core-dashboards
ivanortegaalba
added
minor
| if (this.state.enabled) { | ||
| this.enable(); | ||
| } | ||
| public constructor({ enabled = false }) { |
There was a problem hiding this comment.
I think the refactor here has created a bug. Testing this out I can't open grafana:
I think the constructor should be as before?
public constructor(enabled = false) not
public constructor({ enabled = false })
There was a problem hiding this comment.
Ups, I pushed commits from another branch. From this PR #642. Thanks for noticing, fixed now!
ivanortegaalba
force-pushed
the
iortega/autofit-grid-items
branch
from
add5bd7
to
02dc3f2
Compare
|
🚀 PR was released in |
Problem
When using Grafana on a TV, the users want to have all the widgets available on the screen. However, the screen size can vary between rooms, so they mark "auto size" when playing a playlist to have the dashboard fit vertically in the screen.
When migrating to the dashboard, this feature stopped working.
Solution
To be able to adapt the grid to the available height, we modify the RGL cells height to make sure they don't overflow the bottom of the grid view.
Screenshare.-.2024-03-22.3_12_54.PM.mp4
📦 Published PR as canary version:
4.2.0--canary.658.8522699461.0✨ Test out this PR locally via:
npm install @grafana/scenes@4.2.0--canary.658.8522699461.0 # or yarn add @grafana/scenes@4.2.0--canary.658.8522699461.0