-
Notifications
You must be signed in to change notification settings - Fork 14.4k
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
refactor : Transform SaveModal to typescript #11951
Changes from 4 commits
9661c58
726e7fe
528af0a
56e52c5
d73a17d
ea0ee9e
debcbf0
8b92787
2572a3e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,42 +18,60 @@ | |
*/ | ||
/* eslint-env browser */ | ||
import React from 'react'; | ||
import PropTypes from 'prop-types'; | ||
import { FormControl, FormGroup, Radio } from 'react-bootstrap'; | ||
import Button from 'src/components/Button'; | ||
import { t, CategoricalColorNamespace } from '@superset-ui/core'; | ||
|
||
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, | ||
import { t, CategoricalColorNamespace, JsonResponse } from '@superset-ui/core'; | ||
|
||
import ModalTrigger from 'src/components/ModalTrigger'; | ||
import Checkbox from 'src/components/Checkbox'; | ||
import { | ||
SAVE_TYPE_OVERWRITE, | ||
SAVE_TYPE_NEWDASHBOARD, | ||
} from 'src/dashboard/util/constants'; | ||
|
||
type SaveType = typeof SAVE_TYPE_OVERWRITE | typeof SAVE_TYPE_NEWDASHBOARD; | ||
|
||
type SaveModalProps = { | ||
addSuccessToast: (arg: string) => void; | ||
addDangerToast: (arg: string) => void; | ||
dashboardId: number; | ||
dashboardTitle: string; | ||
dashboardInfo: Record<string, any>; | ||
expandedSlices: object; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you update all instances of |
||
layout: object; | ||
saveType: SaveType; | ||
triggerNode: JSX.Element; | ||
customCss: string; | ||
colorNamespace?: string; | ||
colorScheme?: string; | ||
onSave: (data: any, id: number | string, saveType: SaveType) => void; | ||
canOverwrite: boolean; | ||
shouldPersistRefreshFrequency: boolean; | ||
refreshFrequency: number; | ||
lastModifiedTime: number; | ||
}; | ||
|
||
type SaveModalState = { | ||
saveType: SaveType; | ||
newDashName: string; | ||
duplicateSlices: boolean; | ||
}; | ||
|
||
const defaultProps = { | ||
saveType: SAVE_TYPE_OVERWRITE, | ||
colorNamespace: undefined, | ||
colorScheme: undefined, | ||
shouldPersistRefreshFrequency: true, | ||
rusackas marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
class SaveModal extends React.PureComponent { | ||
constructor(props) { | ||
class SaveModal extends React.PureComponent<SaveModalProps, SaveModalState> { | ||
static defaultProps = defaultProps; | ||
|
||
modal: ModalTrigger | null; | ||
|
||
onSave: Function; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. and all instances of |
||
|
||
constructor(props: SaveModalProps) { | ||
super(props); | ||
this.state = { | ||
saveType: props.saveType, | ||
|
@@ -69,25 +87,25 @@ class SaveModal extends React.PureComponent { | |
this.onSave = this.props.onSave.bind(this); | ||
} | ||
|
||
setModalRef(ref) { | ||
setModalRef(ref: ModalTrigger | null) { | ||
this.modal = ref; | ||
} | ||
|
||
toggleDuplicateSlices() { | ||
toggleDuplicateSlices(val?: boolean | undefined): void { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you don't need the 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? |
||
this.setState(prevState => ({ | ||
duplicateSlices: !prevState.duplicateSlices, | ||
})); | ||
} | ||
|
||
handleSaveTypeChange(event) { | ||
handleSaveTypeChange(event: React.FormEvent<Radio>) { | ||
this.setState({ | ||
saveType: event.target.value, | ||
saveType: (event.target as HTMLInputElement).value as SaveType, | ||
}); | ||
} | ||
|
||
handleNameChange(event) { | ||
handleNameChange(event: React.FormEvent<FormControl>) { | ||
this.setState({ | ||
newDashName: event.target.value, | ||
newDashName: (event.target as HTMLInputElement).value, | ||
saveType: SAVE_TYPE_NEWDASHBOARD, | ||
}); | ||
} | ||
|
@@ -137,17 +155,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: JsonResponse) => { | ||
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}/`; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. oo, was this a bug that typescript caught? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Setting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wow, you calling me a youngin?!? 😆 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
} | ||
}); | ||
this.modal.close(); | ||
this.modal?.close(); | ||
} | ||
} | ||
|
||
|
@@ -207,7 +225,4 @@ class SaveModal extends React.PureComponent { | |
} | ||
} | ||
|
||
SaveModal.propTypes = propTypes; | ||
SaveModal.defaultProps = defaultProps; | ||
|
||
export default SaveModal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
are these changes intentional? I don't remember them before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be
React.CSSProperties
?