etr2460 commented on a change in pull request #11951:
URL:
https://github.com/apache/incubator-superset/pull/11951#discussion_r537660179
##########
File path: superset-frontend/src/dashboard/components/SaveModal.tsx
##########
@@ -27,33 +26,49 @@ import ModalTrigger from '../../components/ModalTrigger';
import Checkbox from '../../components/Checkbox';
import { SAVE_TYPE_OVERWRITE, SAVE_TYPE_NEWDASHBOARD } from
'../util/constants';
-const propTypes = {
- addSuccessToast: PropTypes.func.isRequired,
- addDangerToast: PropTypes.func.isRequired,
- dashboardId: PropTypes.number.isRequired,
- dashboardTitle: PropTypes.string.isRequired,
- dashboardInfo: PropTypes.object.isRequired,
- expandedSlices: PropTypes.object.isRequired,
- layout: PropTypes.object.isRequired,
- saveType: PropTypes.oneOf([SAVE_TYPE_OVERWRITE, SAVE_TYPE_NEWDASHBOARD]),
- triggerNode: PropTypes.node.isRequired,
- customCss: PropTypes.string.isRequired,
- colorNamespace: PropTypes.string,
- colorScheme: PropTypes.string,
- onSave: PropTypes.func.isRequired,
- canOverwrite: PropTypes.bool.isRequired,
- refreshFrequency: PropTypes.number.isRequired,
- lastModifiedTime: PropTypes.number.isRequired,
+type SaveType = typeof SAVE_TYPE_OVERWRITE | typeof SAVE_TYPE_NEWDASHBOARD;
+
+type SaveModalProps = {
+ addSuccessToast: Function;
Review comment:
Instead of `Function` can you define the arguments and return values
expected in the function here too?`
##########
File path: superset-frontend/src/dashboard/components/SaveModal.tsx
##########
@@ -27,33 +26,49 @@ import ModalTrigger from '../../components/ModalTrigger';
import Checkbox from '../../components/Checkbox';
import { SAVE_TYPE_OVERWRITE, SAVE_TYPE_NEWDASHBOARD } from
'../util/constants';
-const propTypes = {
- addSuccessToast: PropTypes.func.isRequired,
- addDangerToast: PropTypes.func.isRequired,
- dashboardId: PropTypes.number.isRequired,
- dashboardTitle: PropTypes.string.isRequired,
- dashboardInfo: PropTypes.object.isRequired,
- expandedSlices: PropTypes.object.isRequired,
- layout: PropTypes.object.isRequired,
- saveType: PropTypes.oneOf([SAVE_TYPE_OVERWRITE, SAVE_TYPE_NEWDASHBOARD]),
- triggerNode: PropTypes.node.isRequired,
- customCss: PropTypes.string.isRequired,
- colorNamespace: PropTypes.string,
- colorScheme: PropTypes.string,
- onSave: PropTypes.func.isRequired,
- canOverwrite: PropTypes.bool.isRequired,
- refreshFrequency: PropTypes.number.isRequired,
- lastModifiedTime: PropTypes.number.isRequired,
+type SaveType = typeof SAVE_TYPE_OVERWRITE | typeof SAVE_TYPE_NEWDASHBOARD;
+
+type SaveModalProps = {
+ addSuccessToast: Function;
+ addDangerToast: Function;
+ dashboardId: number;
+ dashboardTitle: string;
+ dashboardInfo: object;
Review comment:
Prefer `Record<string, any>` to `object`
##########
File path: 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) => {
Review comment:
I believe a `SupersetResponse` type exists, perhaps in
`superset-ui/core`? That would be preferable to `any` here
##########
File path: 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) => {
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}/`;
Review comment:
oo, was this a bug that typescript caught?
##########
File path: superset-frontend/src/dashboard/components/SaveModal.tsx
##########
@@ -69,25 +84,25 @@ class SaveModal extends React.PureComponent {
this.onSave = this.props.onSave.bind(this);
}
- setModalRef(ref) {
+ setModalRef(ref: ModalTrigger | null) {
this.modal = ref;
}
- toggleDuplicateSlices() {
+ toggleDuplicateSlices(): any {
Review comment:
Why does this return `any`? seems like nothing is returned
##########
File path: 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
Review comment:
This is a bit worrisome, can we do this without the `any`?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
