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

[Dashboard][Collapsable Panels] Remove API in favour of props #200090

Closed
Tracked by #190342
Heenawter opened this issue Nov 13, 2024 · 1 comment · Fixed by #200793
Closed
Tracked by #190342

[Dashboard][Collapsable Panels] Remove API in favour of props #200090

Heenawter opened this issue Nov 13, 2024 · 1 comment · Fixed by #200793
Assignees
Labels
Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Project:Collapsable Panels Related to the project for adding collapsable sections to Dashboards. Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas

Comments

@Heenawter
Copy link
Contributor

Heenawter commented Nov 13, 2024

In #195513, we introduced a panel management API for handling stuff like adding a panel, removing a panel, etc. After an initial exploration into #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
getCreationOptions: () => {
  initialLayout: GridLayoutData;
  gridSettings: GridSettings
};
renderPanelContents: (
  panelId: string,
  setDragHandles: (refs: Array<HTMLElement | null>) => void
) => React.ReactNode;
onLayoutChange: (newLayout: GridLayoutData) => void;
layout: GridLayoutData;
gridSettings: GridSettings;
renderPanelContents: (
  panelId: string,
  setDragHandles: (refs: Array<HTMLElement | null>) => 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.

@Heenawter Heenawter added Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Project:Collapsable Panels Related to the project for adding collapsable sections to Dashboards. Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas labels Nov 13, 2024
@Heenawter Heenawter self-assigned this Nov 13, 2024
@elasticmachine
Copy link
Contributor

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this issue Nov 21, 2024
Closes elastic#200090

## Summary

This PR migrates the `GridLayout` component a more traditional React
design using **props** rather than providing an API. This change serves
two purposes:
1. It makes the eventual Dashboard migration easier, since it is more
similar to `react-grid-layout`'s implementation
3. It makes the `GridLayout` component less opinionated by moving the
logic for panel management (i.e. panel placement, etc) to the parent
component.

I tried to keep efficiency in mind for this comparison, and ensured that
we are still keeping the number of rerenders **o a minimum**. This PR
should not introduce **any** extra renders in comparison to the API
version.

### Checklist

- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

There are no risks to this PR, since all work is contained in the
`examples` plugin.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit 5495322)
@Heenawter Heenawter removed the Feature:Dashboard Dashboard related features label Nov 21, 2024
@Heenawter Heenawter added the Feature:Dashboard Dashboard related features label Nov 21, 2024
paulinashakirova pushed a commit to paulinashakirova/kibana that referenced this issue Nov 26, 2024
Closes elastic#200090

## Summary

This PR migrates the `GridLayout` component a more traditional React
design using **props** rather than providing an API. This change serves
two purposes:
1. It makes the eventual Dashboard migration easier, since it is more
similar to `react-grid-layout`'s implementation
3. It makes the `GridLayout` component less opinionated by moving the
logic for panel management (i.e. panel placement, etc) to the parent
component.

I tried to keep efficiency in mind for this comparison, and ensured that
we are still keeping the number of rerenders **o a minimum**. This PR
should not introduce **any** extra renders in comparison to the API
version.

### Checklist

- [x] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

There are no risks to this PR, since all work is contained in the
`examples` plugin.

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Dashboard Dashboard related features impact:high Addressing this issue will have a high level of impact on the quality/strength of our product. loe:medium Medium Level of Effort Project:Collapsable Panels Related to the project for adding collapsable sections to Dashboards. Team:Presentation Presentation Team for Dashboard, Input Controls, and Canvas
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants