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] Remove API in favour of props #200090

Open Heenawter opened 3 days ago

Heenawter commented 3 days ago

In https://github.com/elastic/kibana/pull/195513, we introduced a panel management API for handling stuff like adding a panel, removing a panel, etc. After an initial exploration into https://github.com/elastic/kibana/issues/190446, I found an issue with this approach because we were running into colliding sources of truth.

For example, consider adding a panel. In order to get the new layout information + position of the new panel, I must call the grid layout API's addPanel method - this triggers gridLayoutStateManager.gridLayout$.next to be called, which then tries to call renderPanelContents for the new panel. The problem is, the panels$ array hasn't been updated for the dashboard yet; so it tries to render an embeddable that doesn't yet exist. But I can't update the panels$ array before the add panel method is called, because I don't know what gridData I should use. Conundrum! 🤔

While it is nice to have all of the panel placement logic, etc. contained in the layout engine, rather than fighting with these dual sources of truth, we have decided to convert the GridLayout component from an API-focused implementation to a more traditional props-focused React component. i.e our props will look something like this:

Before After
```typescript getCreationOptions: () => { initialLayout: GridLayoutData; gridSettings: GridSettings }; renderPanelContents: ( panelId: string, setDragHandles: (refs: Array) => void ) => React.ReactNode; onLayoutChange: (newLayout: GridLayoutData) => void; ``` ```typescript layout: GridLayoutData; gridSettings: GridSettings; renderPanelContents: ( panelId: string, setDragHandles: (refs: Array) => void ) => React.ReactNode; onLayoutChange: (newLayout: GridLayoutData) => void; ```

This keeps the implementation more in line with the current react-grid-layout component, as well, which should make the final dashboard swap a little easier.

As a later improvement, we could adjust the panels$ logic in Dashboard so that the grid data and the panels data are tracked seperately - that way, we only have one source of truth for each thing. However, we consider this to be an enhancement, since it would be fundamental change on the Dashboard side.

elasticmachine commented 3 days ago

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