etr2460 commented on a change in pull request #17603:
URL: https://github.com/apache/superset/pull/17603#discussion_r759745837
##########
File path:
superset-frontend/src/SqlLab/components/EstimateQueryCostButton/index.tsx
##########
@@ -26,24 +25,28 @@ import Loading from 'src/components/Loading';
import ModalTrigger from 'src/components/ModalTrigger';
import { EmptyWrapperType } from 'src/components/TableView/TableView';
-const propTypes = {
- dbId: PropTypes.number.isRequired,
- schema: PropTypes.string.isRequired,
- sql: PropTypes.string.isRequired,
- getEstimate: PropTypes.func.isRequired,
- queryCostEstimate: PropTypes.Object,
- selectedText: PropTypes.string,
- tooltip: PropTypes.string,
- disabled: PropTypes.bool,
-};
-const defaultProps = {
- queryCostEstimate: [],
- tooltip: '',
- disabled: false,
-};
+interface EstimateQueryCostButtonProps {
+ dbId: number;
+ schema: string;
+ sql: string;
+ getEstimate: Function;
+ queryCostEstimate: Record<string, any>;
Review comment:
You're saying this is a record, but using an empty array as the default
value. I know that's what was used before, but something probably needs to
change here.
##########
File path:
superset-frontend/src/SqlLab/components/EstimateQueryCostButton/index.tsx
##########
@@ -26,24 +25,28 @@ import Loading from 'src/components/Loading';
import ModalTrigger from 'src/components/ModalTrigger';
import { EmptyWrapperType } from 'src/components/TableView/TableView';
-const propTypes = {
- dbId: PropTypes.number.isRequired,
- schema: PropTypes.string.isRequired,
- sql: PropTypes.string.isRequired,
- getEstimate: PropTypes.func.isRequired,
- queryCostEstimate: PropTypes.Object,
- selectedText: PropTypes.string,
- tooltip: PropTypes.string,
- disabled: PropTypes.bool,
-};
-const defaultProps = {
- queryCostEstimate: [],
- tooltip: '',
- disabled: false,
-};
+interface EstimateQueryCostButtonProps {
+ dbId: number;
+ schema: string;
+ sql: string;
+ getEstimate: Function;
+ queryCostEstimate: Record<string, any>;
+ selectedText?: string;
+ tooltip?: string;
+ disabled: boolean;
+}
-const EstimateQueryCostButton = props => {
- const { cost } = props.queryCostEstimate;
+const EstimateQueryCostButton = ({
+ dbId,
+ schema,
+ sql,
+ getEstimate,
+ queryCostEstimate = [],
Review comment:
this probably should be
```suggestion
queryCostEstimate = {},
```
##########
File path:
superset-frontend/src/SqlLab/components/EstimateQueryCostButton/index.tsx
##########
@@ -26,24 +25,28 @@ import Loading from 'src/components/Loading';
import ModalTrigger from 'src/components/ModalTrigger';
import { EmptyWrapperType } from 'src/components/TableView/TableView';
-const propTypes = {
- dbId: PropTypes.number.isRequired,
- schema: PropTypes.string.isRequired,
- sql: PropTypes.string.isRequired,
- getEstimate: PropTypes.func.isRequired,
- queryCostEstimate: PropTypes.Object,
- selectedText: PropTypes.string,
- tooltip: PropTypes.string,
- disabled: PropTypes.bool,
-};
-const defaultProps = {
- queryCostEstimate: [],
- tooltip: '',
- disabled: false,
-};
+interface EstimateQueryCostButtonProps {
+ dbId: number;
+ schema: string;
+ sql: string;
+ getEstimate: Function;
+ queryCostEstimate: Record<string, any>;
+ selectedText?: string;
+ tooltip?: string;
+ disabled: boolean;
Review comment:
i _think_ disabled should be `disabled?: boolean` so that the caller
doesn't need to specify it?
##########
File path:
superset-frontend/src/SqlLab/components/EstimateQueryCostButton/index.tsx
##########
@@ -54,20 +57,20 @@ const EstimateQueryCostButton = props => {
);
const onClick = () => {
Review comment:
hmm, do we even need the `onClick` function if all it does is call
`getEstimate`? Seems like we can put `getEstimate` directly on the button's
onClick prop
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
