Skip to content

Fix Send to Active Notebook functionality#341

Merged
mofojed merged 4 commits intodeephaven:mainfrom
mofojed:fix-send-to-notebook
Dec 9, 2021
Merged

Fix Send to Active Notebook functionality#341
mofojed merged 4 commits intodeephaven:mainfrom
mofojed:fix-send-to-notebook

Conversation

@mofojed
Copy link
Member

@mofojed mofojed commented Dec 9, 2021

  • Added getLastUsedPanelOfTypes to PanelManager (will be needed for Enterprise)
  • Clean up some of the TypeScript types

- Added `getLastUsedPanelOfTypes` to PanelManager (will be needed for Enterprise)
- Clean up some of the TypeScript types
@mofojed mofojed added bug Something isn't working web-client-ui labels Dec 9, 2021
@mofojed mofojed added this to the December 2021 milestone Dec 9, 2021
@mofojed mofojed requested a review from vbabich December 9, 2021 15:01
@mofojed mofojed self-assigned this Dec 9, 2021
registerComponent(
NotebookPanel.COMPONENT,
(NotebookPanel as unknown) as ComponentType
(FileExplorerPanel as unknown) as PanelComponentType
Copy link
Member Author

Choose a reason for hiding this comment

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

@mattrunyon any idea how to get TypeScript typings correct so we don't need to explicitly convert this one? I'm having a hard time getting TypeScript to co-operate here.

Copy link
Member Author

Choose a reason for hiding this comment

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

The error:

Argument of type 'ConnectedComponent<typeof FileExplorerPanel, Omit<ClassAttributes<FileExplorerPanel> & PanelProps & { localDashboardId: string; metadata?: Record<...> | undefined; } & StateProps & DispatchProp<...>, "dispatch" | ... 2 more ... | "fileStorage"> & PanelProps & { ...; }>' is not assignable to parameter of type 'PanelComponentType<ComponentClass<{}, any>, Omit<ClassAttributes<FileExplorerPanel> & PanelProps & { localDashboardId: string; metadata?: Record<...> | undefined; } & StateProps & DispatchProp<...>, "dispatch" | ... 2 more ... | "fileStorage"> & PanelProps & { ...; }>'.
  Type 'ConnectedComponent<typeof FileExplorerPanel, Omit<ClassAttributes<FileExplorerPanel> & PanelProps & { localDashboardId: string; metadata?: Record<...> | undefined; } & StateProps & DispatchProp<...>, "dispatch" | ... 2 more ... | "fileStorage"> & PanelProps & { ...; }>' is not assignable to type 'FunctionComponent<{}>'.
    Types of parameters 'props' and 'props' are incompatible.
      Type '{ children?: ReactNode; }' is not assignable to type 'Omit<ClassAttributes<FileExplorerPanel> & PanelProps & { localDashboardId: string; metadata?: Record<string, unknown> | undefined; } & StateProps & DispatchProp<...>, "dispatch" | ... 2 more ... | "fileStorage"> & PanelProps & { ...; }'.
        Type '{ children?: ReactNode; }' is missing the following properties from type 'Omit<ClassAttributes<FileExplorerPanel> & PanelProps & { localDashboardId: string; metadata?: Record<string, unknown> | undefined; } & StateProps & DispatchProp<...>, "dispatch" | ... 2 more ... | "fileStorage">': glContainer, glEventHub, localDashboardIdts(2345)

@codecov
Copy link

codecov bot commented Dec 9, 2021

Codecov Report

Merging #341 (c1cf87f) into main (cdce34d) will increase coverage by 6.48%.
The diff coverage is 55.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            main     #341      +/-   ##
=========================================
+ Coverage   5.94%   12.42%   +6.48%     
=========================================
  Files          2       17      +15     
  Lines        202     1199     +997     
  Branches      35      255     +220     
=========================================
+ Hits          12      149     +137     
- Misses       190      989     +799     
- Partials       0       61      +61     
Flag Coverage Δ
unit 12.42% <55.00%> (+6.48%) ⬆️

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

Impacted Files Coverage Δ
...ackages/dashboard-core-plugins/src/ChartPlugin.tsx 0.00% <ø> (ø)
...kages/dashboard-core-plugins/src/ConsolePlugin.tsx 0.00% <0.00%> (ø)
...ckages/dashboard-core-plugins/src/FilterPlugin.tsx 0.00% <ø> (ø)
packages/dashboard-core-plugins/src/GridPlugin.tsx 0.00% <ø> (ø)
...ages/dashboard-core-plugins/src/MarkdownPlugin.tsx 0.00% <ø> (ø)
packages/dashboard/src/DashboardLayout.tsx 50.58% <ø> (ø)
...oard-core-plugins/src/panels/FileExplorerPanel.tsx 5.81% <28.57%> (ø)
packages/dashboard/src/PanelManager.ts 48.61% <77.77%> (ø)
packages/dashboard/src/DashboardPlugin.ts 100.00% <100.00%> (ø)
packages/console/src/StoragePropTypes.js
... and 17 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cdce34d...c1cf87f. Read the comment docs.

panelManager.getLastUsedPanelOfTypes([TestComponentA, TestComponentB])
).toBe(panelB);
expect(
panelManager.getLastUsedPanelOfTypes([TestComponentB, TestComponentB])
Copy link
Collaborator

Choose a reason for hiding this comment

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

I made a typo in the original code, this was supposed to be

Suggested change
panelManager.getLastUsedPanelOfTypes([TestComponentB, TestComponentB])
panelManager.getLastUsedPanelOfTypes([TestComponentB, TestComponentA])

types.some(
type =>
panel instanceof type ||
(isWrappedComponent(type) &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we get away with a simple panel => panel instanceof type check here? just pass unwrapped components to getLastUsedPanelOfTypes and skip the isWrappedComponent check? I didn't think of this solution when I added this on the enterprise side, then I saw you had it simplified in DHC. Then I guess remove | WrappedComponentType<C, P> from the PanelComponentType definition too.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've fixed this, but for posterity, we still need the WrappedComponentType for registerComponent anyway, so may as well make it work right!

- Flip the P and C template parameters around, so C can include the props type P
- Include the template parameters in the registerComponent and getLastUsedPanelOfTypes
@mofojed mofojed requested a review from vbabich December 9, 2021 19:28
Copy link
Collaborator

@vbabich vbabich left a comment

Choose a reason for hiding this comment

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

Nice!

@mofojed mofojed merged commit e7c518c into deephaven:main Dec 9, 2021
@mofojed mofojed deleted the fix-send-to-notebook branch December 9, 2021 19:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working web-client-ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants