search

elastic / kibana

Your window into the Elastic Stack
https://www.elastic.co/products/kibana
Other
19.65k stars 8.23k forks source link

[Dashboard][Collapsable Panels] Fix panel order edge case #191306

Open ThomThomson opened 2 months ago

ThomThomson commented 2 months ago

The kbn-grid-layout algorithm defined in packages/kbn-grid-layout/grid/resolve_grid_row.ts is quite different from the one in react grid layout. This causes an edge case when a panel is resized into colliding with multiple panels at once. This results in the following undesired behaviour:

react grid layout kbn grid layout
Image Image

In the gif above, notice how the relative orders of panels 6, 7, and 8 are not maintained in kbn grid layout. Note that this only happens when a single operation causes collisions with multiple panels, which makes it quite rare.

elasticmachine commented 2 months ago

Pinging @elastic/kibana-presentation (Team:Presentation)

ThomThomson commented 2 months ago

Here is a breakdown of what happens in this case.

  1. Panel1 is expanded one space to the right
  2. Panel1 is colliding with panel5
  3. The y overlap between panel1 and panel5 is 4
  4. panel5 is moved down 4 spaces...
  5. panel6 checks for collisions... panel5 is now completely below panel6 so there are no collisions. panel6 stays where it is.
  6. Same for panel7 and panel8.
  7. The grid is compacted and all the negative space above those three panels is removed, pushing them to the top.

In React grid layout, the algorithm is slower but does not suffer from this issue. Instead of pushing the panel down by the full amount of overlap in step 4 it moves the panel down by just one space, and then recurses.

Best case scenario here, we can keep our faster implementation while removing this edge case. In the worst case scenario, we can use an approach similar to the recursive one in React Grid Layout.

Heenawter commented 1 month ago

Noting that this feels like it's happening pretty often as I work on the logic for placing a new panel - specifically with the findTopLeftMostOpenSpace placement strategy, which relies on calling resolveGridRow to push panels down when we find a valid space for the panel to go. So I am assigning this a higher impact.

https://github.com/user-attachments/assets/32262d42-db0d-466a-b630-0871442836e4

In the above video, you'll see that panels 7 and 8 get pushed above panel 5 after adding the new panel, because, after resolving the collision between panel 5 and the new panel 11 (which pushes panel 5 below panel 11), panels 7 and 8 no longer have collisions - so instead of getting moved down along with panel 5, they get pushed up to fill the empty space when the row gets compacted.

Note that we could modify the panel placement strategy to not allow panels that are taller than the open space - i.e. in that case, we would never have to push other panels down to accommodate. However, this would be a change in behaviour from Dashboard's panel placement logic, and I am trying to keep this identical for now. This would also make the panel placement strategy a lot less flexible, IMO