refactor : Transform SaveModal to typescript #11951

maloun96 · 2020-12-07T13:19:58Z

SUMMARY

Transform SaveModal to Typescript

maloun96 · 2020-12-07T13:22:38Z

codecov-io · 2020-12-07T13:55:30Z

Codecov Report

Merging #11951 (2572a3e) into master (a7bba92) will decrease coverage by 0.80%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master   #11951      +/-   ##
==========================================
- Coverage   64.45%   63.64%   -0.81%     
==========================================
  Files         937      942       +5     
  Lines       45319    45826     +507     
  Branches     4317     4400      +83     
==========================================
- Hits        29211    29168      -43     
- Misses      15948    16482     +534     
- Partials      160      176      +16

Flag	Coverage Δ
cypress	`?`
javascript	`62.72% <33.33%> (-0.27%)`	⬇️
python	`64.19% <ø> (+0.01%)`	⬆️

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

Impacted Files	Coverage Δ
...uperset-frontend/src/components/Checkbox/index.tsx	`100.00% <ø> (ø)`
...et-frontend/src/dashboard/components/SaveModal.tsx	`19.23% <33.33%> (ø)`
superset-frontend/src/dashboard/App.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/dashboard/index.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/setup/setupColors.js	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/chart/ChartContainer.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
...et-frontend/src/dashboard/containers/Dashboard.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
...-frontend/src/visualizations/presets/MainPreset.js	`0.00% <0.00%> (-100.00%)`	⬇️
...tend/src/dashboard/containers/DashboardBuilder.jsx	`0.00% <0.00%> (-100.00%)`	⬇️
superset-frontend/src/setup/setupFormatters.ts	`0.00% <0.00%> (-75.00%)`	⬇️
... and 102 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 a7bba92...2572a3e. Read the comment docs.

etr2460 · 2020-12-07T16:49:19Z

superset-frontend/src/dashboard/components/SaveModal.tsx

+type SaveType = typeof SAVE_TYPE_OVERWRITE | typeof SAVE_TYPE_NEWDASHBOARD;
+
+type SaveModalProps = {
+  addSuccessToast: Function;


Instead of Function can you define the arguments and return values expected in the function here too?`

etr2460 · 2020-12-07T16:52:03Z

superset-frontend/src/dashboard/components/SaveModal.tsx

+  addDangerToast: Function;
+  dashboardId: number;
+  dashboardTitle: string;
+  dashboardInfo: object;


Prefer Record<string, any> to object

etr2460 · 2020-12-07T16:52:40Z

superset-frontend/src/dashboard/components/SaveModal.tsx

    this.modal = ref;
  }

-  toggleDuplicateSlices() {
+  toggleDuplicateSlices(): any {


Why does this return any? seems like nothing is returned

etr2460 · 2020-12-07T16:53:15Z

superset-frontend/src/dashboard/components/SaveModal.tsx

@@ -116,7 +131,7 @@ class SaveModal extends React.PureComponent {
    // check refresh frequency is for current session or persist
    const refreshFrequency = shouldPersistRefreshFrequency
      ? currentRefreshFrequency
-      : dashboardInfo.metadata.refresh_frequency; // eslint-disable camelcase
+      : (dashboardInfo as any).metadata.refresh_frequency; // eslint-disable camelcase


This is a bit worrisome, can we do this without the any?

etr2460 · 2020-12-07T16:53:45Z

superset-frontend/src/dashboard/components/SaveModal.tsx

@@ -137,17 +152,17 @@ class SaveModal extends React.PureComponent {
        t('You must pick a name for the new dashboard'),
      );
    } else {
-      this.onSave(data, dashboardId, saveType).then(resp => {
+      this.onSave(data, dashboardId, saveType).then((resp: any) => {


I believe a SupersetResponse type exists, perhaps in superset-ui/core? That would be preferable to any here

etr2460 · 2020-12-07T16:54:02Z

superset-frontend/src/dashboard/components/SaveModal.tsx

        if (
          saveType === SAVE_TYPE_NEWDASHBOARD &&
          resp &&
          resp.json &&
          resp.json.id
        ) {
-          window.location = `/superset/dashboard/${resp.json.id}/`;
+          window.location.href = `/superset/dashboard/${resp.json.id}/`;


oo, was this a bug that typescript caught?

Setting location is equivalent to setting location.href. Old JavaScript programmers would know 😉

Wow, you calling me a youngin?!? 😆

Well, back when I started building websites, there wasn't even JavaScript yet! Or CSS! Gather round, let me spin you a yarn about the old days. You see, there was this browser called Mosaic...

... hey, are you kids even listening?

etr2460 · 2020-12-08T19:09:44Z

superset-frontend/src/dashboard/components/SaveModal.tsx

    this.modal = ref;
  }

-  toggleDuplicateSlices() {
+  toggleDuplicateSlices(val?: boolean | undefined): void {


you don't need the | undefined if you already have a ?:

Although, I wonder why you added an argument here since it seems like the function doesn't actually use it? Is that because you pass it into a component that expects a function with this arg?

etr2460 · 2020-12-08T19:09:56Z

superset-frontend/src/dashboard/components/SaveModal.tsx

+  dashboardId: number;
+  dashboardTitle: string;
+  dashboardInfo: Record<string, any>;
+  expandedSlices: object;


Could you update all instances of object?

etr2460 · 2020-12-08T19:10:07Z

superset-frontend/src/dashboard/components/SaveModal.tsx

+
+  modal: ModalTrigger | null;
+
+  onSave: Function;


and all instances of Function? Thanks!

etr2460 · 2020-12-08T19:10:42Z

superset-frontend/src/components/Checkbox/index.tsx

+  onChange: (val?: boolean) => void;
+  style?: object;


are these changes intentional? I don't remember them before

If it is intentional (I'm not sure either), it'd be the only remaining instance of object so maybe we can touch that up too?

Shouldn't it be React.CSSProperties?

maloun96 · 2020-12-09T07:24:53Z

@etr2460 Refactored.

etr2460

One more comment from me (and also @rusackas/@ktmud's comment too)

etr2460 · 2020-12-09T19:28:51Z

superset-frontend/src/dashboard/components/SaveModal.tsx

@@ -185,7 +207,7 @@ class SaveModal extends React.PureComponent {
            <div className="m-l-25 m-t-5">
              <Checkbox
                checked={this.state.duplicateSlices}
-                onChange={this.toggleDuplicateSlices}
+                onChange={() => this.toggleDuplicateSlices}


wondering why this was needed, seems like it might break it (now that toggleDuplicateSlices isn't actually being called I think)

rusackas

LGTM! Thanks all for the comments, the fixes, and the reminiscing about the olden days.

etr2460

lgtm with one final question

superset-frontend/src/dashboard/components/SaveModal.tsx

rusackas · 2020-12-11T07:01:58Z

I think all questions/issues have been resolved! Thanks, everyone!

Transform SaveModal to typescript

9661c58

pull-request-size bot added the size/M label Dec 7, 2020

Revert title

726e7fe

maloun96 changed the title ~~Transform SaveModal to typescript~~ refactor : Transform SaveModal to typescript Dec 7, 2020

etr2460 requested changes Dec 7, 2020

View reviewed changes

junlincc added the flexiana label Dec 7, 2020

maloun96 added 2 commits December 8, 2020 09:58

Resolve code review

528af0a

Refactor

56e52c5

etr2460 reviewed Dec 8, 2020

View reviewed changes

Refactor

d73a17d

pull-request-size bot added size/L and removed size/M labels Dec 9, 2020

etr2460 reviewed Dec 9, 2020

View reviewed changes

Victor Malai added 3 commits December 9, 2020 22:02

Change

ea0ee9e

Call method

debcbf0

Fix test

8b92787

rusackas approved these changes Dec 10, 2020

View reviewed changes

etr2460 approved these changes Dec 10, 2020

View reviewed changes

superset-frontend/src/dashboard/components/SaveModal.tsx Show resolved Hide resolved

graceguo-supercat reviewed Dec 10, 2020

View reviewed changes

superset-frontend/src/dashboard/components/SaveModal.tsx Outdated Show resolved Hide resolved

ShouldPersist default false

2572a3e

rusackas merged commit 12d9d1e into apache:master Dec 11, 2020

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 1.0.0 labels Mar 12, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor : Transform SaveModal to typescript #11951

refactor : Transform SaveModal to typescript #11951

maloun96 commented Dec 7, 2020

maloun96 commented Dec 7, 2020

codecov-io commented Dec 7, 2020 •

edited

Loading

etr2460 Dec 7, 2020

etr2460 Dec 7, 2020

etr2460 Dec 7, 2020

etr2460 Dec 7, 2020

etr2460 Dec 7, 2020

etr2460 Dec 7, 2020

ktmud Dec 9, 2020 •

edited

Loading

etr2460 Dec 9, 2020

rusackas Dec 10, 2020

etr2460 Dec 8, 2020

etr2460 Dec 8, 2020

etr2460 Dec 8, 2020

etr2460 Dec 8, 2020

rusackas Dec 9, 2020

ktmud Dec 9, 2020 •

edited

Loading

maloun96 commented Dec 9, 2020

etr2460 left a comment

etr2460 Dec 9, 2020 •

edited

Loading

rusackas left a comment

etr2460 left a comment

rusackas commented Dec 11, 2020

refactor : Transform SaveModal to typescript #11951

refactor : Transform SaveModal to typescript #11951

Conversation

maloun96 commented Dec 7, 2020

SUMMARY

maloun96 commented Dec 7, 2020

codecov-io commented Dec 7, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

maloun96 commented Dec 9, 2020

etr2460 left a comment

Choose a reason for hiding this comment

etr2460 Dec 9, 2020 • edited Loading

Choose a reason for hiding this comment

rusackas left a comment

Choose a reason for hiding this comment

etr2460 left a comment

Choose a reason for hiding this comment

rusackas commented Dec 11, 2020

codecov-io commented Dec 7, 2020 •

edited

Loading

ktmud Dec 9, 2020 •

edited

Loading

ktmud Dec 9, 2020 •

edited

Loading

etr2460 Dec 9, 2020 •

edited

Loading