Skip to content

feat: Mount layout panels inside the main react tree#1229

Merged
mattrunyon merged 8 commits intodeephaven:mainfrom
mattrunyon:golden-layout-react
May 17, 2023
Merged

feat: Mount layout panels inside the main react tree#1229
mattrunyon merged 8 commits intodeephaven:mainfrom
mattrunyon:golden-layout-react

Conversation

@mattrunyon
Copy link
Copy Markdown
Collaborator

@mattrunyon mattrunyon commented Apr 15, 2023

This will be useful for any providers/contexts we want to use without needing to wrap each panel in the provider and synchronize them (works w/ redux, becomes painful with react providers like the spectrum provider)

Tested interactions

  • Opening panels (tables and charts) by creating the object in a query
  • Creating a different panel w/ the same name as an existing panel so the panel contents get replaced
  • Reloading the page and checking layout persisted
  • Rearranging panels
  • Resizing panels
  • Opening notebook in preview mode/regular mode
  • Promoting notebooks from file explorer and by double clicking the preview tab header
  • Maximize/minimize stacks (groups of panels)
  • Running code from notebooks
  • Opening panels from global panels menu
  • Opening panels from console panels menu
  • Opening/focusing panel from pill button in console after creating
  • Double clicking on the pill button in the console after creating a table/chart
  • Linker

@mattrunyon mattrunyon self-assigned this Apr 15, 2023
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 15, 2023

Codecov Report

Merging #1229 (f549e60) into main (d304300) will increase coverage by 0.00%.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##             main    #1229   +/-   ##
=======================================
  Coverage   45.89%   45.89%           
=======================================
  Files         492      492           
  Lines       34371    34373    +2     
  Branches     8566     8565    -1     
=======================================
+ Hits        15775    15776    +1     
- Misses      18545    18546    +1     
  Partials       51       51           
Flag Coverage Δ
unit 45.89% <50.00%> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/dashboard/src/DashboardPlugin.ts 18.18% <ø> (ø)
packages/dashboard/src/DashboardLayout.tsx 47.27% <50.00%> (+0.05%) ⬆️

@mattrunyon mattrunyon marked this pull request as ready for review April 15, 2023 18:32
@mattrunyon mattrunyon requested a review from mofojed April 15, 2023 18:32
Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting an issue when opening a panel - if I click a table twice from the console, it opens briefly and then opens into a blank panel (on the second click)

Comment thread packages/golden-layout/src/utils/EventEmitter.ts Outdated
Comment thread packages/golden-layout/src/utils/ReactComponentHandler.tsx
Comment thread packages/golden-layout/src/utils/ReactComponentHandler.tsx Outdated
Comment thread packages/dashboard/src/DashboardLayout.tsx Outdated
Comment thread packages/golden-layout/src/LayoutManager.ts
@mattrunyon
Copy link
Copy Markdown
Collaborator Author

Fixed the double clicking a pill button in the console not opening the panel properly. It was an issue of the events triggering rapidly. The first event would open the panel and the 2nd event would replace that panel. However, the layoutConfig did not change between these 2 events, so DashboardLayout did not re-render. The panel was there, just needed something to trigger DashboardLayout re-render. I added an event any time the layout manager adds/removes a react child which is used to update DashboardLayout.

Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still running into an issue - it may be because of the switch from addChild/removeChild with replaceChild? Perhaps we should just change replace to do the add/remove? It may be because of something else. But anyway, issue is really easy to reproduce:

  1. Create a plot
  2. Open the plot from the button
  3. Click the plot button again to "open" it again
  4. Close the plot
  5. Open the plot with the button again

Expected Results:
4) No errors, area previously occupied by the plot is reclaimed
5) Plot re-opens

Actual Results:
4) Error in the logs, area previously occupied by the plot just goes black
5) Plot will not re-open

image

2023-05-04-092131_support_logs.zip

Comment thread packages/golden-layout/src/utils/ReactComponentHandler.tsx Outdated
@mattrunyon mattrunyon requested a review from mofojed May 5, 2023 22:56
Copy link
Copy Markdown
Member

@mofojed mofojed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but let's wait til Vlad's changes are in and we publish that version, then this will be in the next version.

mofojed
mofojed previously approved these changes May 16, 2023
@mattrunyon mattrunyon requested a review from mofojed May 16, 2023 20:29
@mattrunyon mattrunyon merged commit f8f8d61 into deephaven:main May 17, 2023
@mattrunyon mattrunyon deleted the golden-layout-react branch May 17, 2023 18:37
@github-actions github-actions Bot locked and limited conversation to collaborators May 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants