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

Disallow drag and drop a row inside another uncollapsed row #669

Closed mdvictor closed 7 months ago

mdvictor commented 7 months ago

https://github.com/grafana/scenes/assets/36818606/62a14da1-dd64-4a4f-87cc-05ce0405666a

This PR fixes the dashboard crashing when a row is dragged within the area of another uncollapsed row. It's not the most elegant solution, so I'm open to feedback if there is a better way to achieve this, but I think this is the simplest way to fix the bug without adding a lot of complexity, knowing that this will need to be revisited having invalidation in mind.

I think this needs to be further worked on, especially on UX side of things to let users know that they can't have nested rows (e.g.: highlight the uncollapsed row area, similar to how we do to repeated panels and disallow dropping if dragged element is a row). My knowledge of RGL is limited and I couldn't find a proper way to invalidate a drop, which makes me believe that the only way to do it would be to update the layout with the invalid state and then re-update with the old state.

The idea here is to return early if we detect after a drag and drop that we are in a row within a row scenario and force render the grid layout with the old layout. Since there is no actual layout change in the grid props, we need to force the render through the grid layout key prop.

TODO: tests

mdvictor commented 7 months ago

I'm still working on an alternative where we would store the old children state onDragStart and return early in onDragStop if state is invalid which would call onLayoutChange and if an old state is present it would reset to that, which is still flickering, but I'm still looking into it.

mdvictor commented 7 months ago

https://github.com/grafana/scenes/assets/36818606/71d106c3-89a0-486c-886b-f820c18b0199

Might've found a better way of doing this with what I mentioned above that doesn't flicker 🎉 Will push that to another branch as well