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-core-plugins typescript conversion #693

Merged
merged 31 commits into from Aug 17, 2022

Conversation

Zhou-Ziheng
Copy link
Contributor

Fixes #671

@Zhou-Ziheng Zhou-Ziheng self-assigned this Jul 29, 2022
@Zhou-Ziheng Zhou-Ziheng added the enhancement New feature or request label Jul 29, 2022
@Zhou-Ziheng Zhou-Ziheng changed the title 671 plugins ts dashboard-core-plugins typescript conversion Jul 29, 2022
@Zhou-Ziheng Zhou-Ziheng marked this pull request as ready for review August 3, 2022 21:41
@@ -46,6 +47,7 @@
"@fortawesome/react-fontawesome": "^0.1.18",
"classnames": "^2.3.1",
"deep-equal": "^2.0.5",
"jszip": "^3.10.0",
Copy link
Member

Choose a reason for hiding this comment

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

As mentioned in another comment, we should be pinned to 3.2.2 for jszip.

packages/dashboard-core-plugins/package.json Show resolved Hide resolved
packages/dashboard-core-plugins/package.json Show resolved Hide resolved
packages/chart/src/Chart.tsx Outdated Show resolved Hide resolved
packages/chart/src/ChartModel.ts Outdated Show resolved Hide resolved
packages/code-studio/src/main/WidgetUtils.ts Outdated Show resolved Hide resolved
packages/code-studio/src/styleguide/ContextMenus.tsx Outdated Show resolved Hide resolved
| ContextAction[]
| (() => ContextAction[])
| (() => Promise<ContextAction[]>);
actions: ResolvableContextAction[];
Copy link
Member

Choose a reason for hiding this comment

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

Should be ResolvableContextAction | ResolvableContextAction[]

packages/console/src/command-history/CommandHistory.tsx Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Aug 8, 2022

Codecov Report

Merging #693 (8deef88) into main (314cd34) will decrease coverage by 0.17%.
The diff coverage is 29.51%.

@@            Coverage Diff             @@
##             main     #693      +/-   ##
==========================================
- Coverage   35.93%   35.75%   -0.18%     
==========================================
  Files         399      400       +1     
  Lines       30093    30189      +96     
  Branches     7326     7389      +63     
==========================================
- Hits        10814    10795      -19     
- Misses      19179    19303     +124     
+ Partials      100       91       -9     
Flag Coverage Δ
unit 35.75% <29.51%> (-0.18%) ⬇️

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

Impacted Files Coverage Δ
packages/chart/src/Chart.tsx 0.00% <0.00%> (ø)
packages/chart/src/isFigureChartModel.ts 0.00% <0.00%> (ø)
packages/code-studio/src/main/SessionUtils.ts 0.00% <ø> (ø)
packages/code-studio/src/main/WidgetUtils.ts 6.45% <ø> (ø)
packages/components/src/ButtonOld.tsx 0.00% <ø> (ø)
packages/components/src/TimeSlider.tsx 59.64% <ø> (ø)
...mponents/src/context-actions/ContextActionUtils.ts 51.68% <0.00%> (-1.19%) ⬇️
.../components/src/context-actions/ContextActions.tsx 2.59% <0.00%> (-0.26%) ⬇️
packages/console/src/notebook/ScriptEditor.tsx 0.00% <ø> (ø)
.../dashboard-core-plugins/src/ChartBuilderPlugin.tsx 6.25% <0.00%> (ø)
... and 58 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

packages/chart/src/Chart.tsx Outdated Show resolved Hide resolved
packages/console/src/command-history/CommandHistory.tsx Outdated Show resolved Hide resolved
packages/console/src/monaco/MonacoCompletionProvider.tsx Outdated Show resolved Hide resolved
packages/console/src/notebook/ScriptEditor.tsx Outdated Show resolved Hide resolved
Copy link
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.

There's still a few test files that are still in jsx format - was there difficulty in converting these?

packages/dashboard-core-plugins/src/redux/actions.js (should be deleted, .ts file already exists)
packages/dashboard-core-plugins/src/redux/selectors.js
packages/dashboard-core-plugins/src/panels/Panel.test.jsx
packages/dashboard-core-plugins/src/panels/IrisGridPanel.test.jsx
packages/dashboard-core-plugins/src/panels/PanelContextMenu.test.jsx
packages/dashboard-core-plugins/src/panels/ConsolePanel.test.jsx
packages/dashboard-core-plugins/src/panels/MarkdownPanel.test.jsx

packages/dashboard-core-plugins/src/redux/actions.ts Outdated Show resolved Hide resolved
packages/dashboard-core-plugins/src/redux/actions.ts Outdated Show resolved Hide resolved
packages/dashboard-core-plugins/src/redux/actions.ts Outdated Show resolved Hide resolved
packages/iris-grid/src/IrisGridUtils.ts Outdated Show resolved Hide resolved
packages/jsapi-utils/src/TableUtils.ts Outdated Show resolved Hide resolved
import { PanelComponent } from '@deephaven/dashboard';
import { LinkColumn } from '../linker/LinkerUtils';

export type ColumnSelectionValidator = (
Copy link
Member

Choose a reason for hiding this comment

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

This file should probably be in the linker folder... or just even defined in LinkerUtils.


export type ColumnSelectionValidator = (
panel: PanelComponent,
tableColumn?: Partial<LinkColumn>
Copy link
Member

Choose a reason for hiding this comment

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

I think instead of using Partial<LinkColumn>, you want to define a new type, LinkColumnSelection = { name: string, type?: string } and use that.

With Partial, you then also have to check that name is defined, which it always will be.

@@ -550,7 +551,7 @@ export class Linker extends Component<LinkerProps, LinkerState> {
case ToolType.LINKER:
setDashboardColumnSelectionValidator(
localDashboardId,
this.isColumnSelectionValid
this.isColumnSelectionValid as ColumnSelectionValidator
Copy link
Member

Choose a reason for hiding this comment

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

Instead I think isColumnSelectionValid should also take a Partial<LinkColumn> rather than just LinkColumn - you'll need to put the appropriate checks for undefineds in there as well.

In particular - in LinkerUtils, there's a check on the columnType, checking for null:

    // Null columnType in ending link point allows linking to any type
    const isCompatibleType =
      endColumnType === null ||
      TableUtils.isCompatibleType(startColumnType, endColumnType);

@@ -354,7 +353,7 @@ export class Linker extends Component<LinkerProps, LinkerState> {
'endPanel no longer exists, ignoring unsetFilterValue',
panelId
);
} else if (isLinkablePanel(endPanel)) {
} else if (isLinkablePanel(endPanel) && columnType != null) {
Copy link
Member

Choose a reason for hiding this comment

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

unsetFilterValue should just allow null columnType instead. By checking for null here you're changing one of the code paths.

Suggested change
} else if (isLinkablePanel(endPanel) && columnType != null) {
} else if (isLinkablePanel(endPanel)) {

@Zhou-Ziheng Zhou-Ziheng merged commit 951179d into deephaven:main Aug 17, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Aug 17, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Convert packages/dashboard-core-plugins to typescript
3 participants