etr2460 commented on a change in pull request #17202:
URL: https://github.com/apache/superset/pull/17202#discussion_r741599470
##########
File path:
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricOption.jsx
##########
@@ -32,7 +32,7 @@ const propTypes = {
columns: PropTypes.arrayOf(columnType),
savedMetricsOptions: PropTypes.arrayOf(savedMetricType),
savedMetric: savedMetricType,
- datasourceType: PropTypes.string,
+ datasource: PropTypes.object.isRequired,
Review comment:
same comment about if datasource is always here
##########
File path:
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
##########
@@ -397,30 +411,26 @@ export default class AdhocMetricEditPopover extends
React.PureComponent {
key={EXPRESSION_TYPES.SQL}
tab={t('Custom SQL')}
Review comment:
same comment here
##########
File path:
superset-frontend/src/explore/components/controls/MetricControl/MetricDefinitionValue.jsx
##########
@@ -34,7 +34,7 @@ const propTypes = {
savedMetrics: PropTypes.arrayOf(savedMetricType),
savedMetricsOptions: PropTypes.arrayOf(savedMetricType),
multi: PropTypes.bool,
- datasourceType: PropTypes.string,
+ datasource: PropTypes.object.isRequired,
Review comment:
same comment
##########
File path:
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
##########
@@ -370,7 +380,11 @@ export default class AdhocMetricEditPopover extends
React.PureComponent {
/>
</FormItem>
</Tabs.TabPane>
- <Tabs.TabPane key={EXPRESSION_TYPES.SIMPLE} tab={t('Simple')}>
+ <Tabs.TabPane
+ key={EXPRESSION_TYPES.SIMPLE}
+ tab={t('Simple')}
Review comment:
should we add a tooltip on the tab with details on why it's disabled?
##########
File path:
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricPopoverTrigger.tsx
##########
@@ -33,7 +33,7 @@ export type AdhocMetricPopoverTriggerProps = {
columns: { column_name: string; type: string }[];
savedMetricsOptions: savedMetricType[];
savedMetric: savedMetricType;
- datasourceType: string;
+ datasource: Datasource;
Review comment:
same comment about if this should be `datasource: Datasource` or
`datasource?: Datasource`
##########
File path:
superset-frontend/src/explore/components/controls/MetricControl/MetricsControl.jsx
##########
@@ -48,7 +48,7 @@ const propTypes = {
isLoading: PropTypes.bool,
multi: PropTypes.bool,
clearable: PropTypes.bool,
- datasourceType: PropTypes.string,
+ datasource: PropTypes.object.isRequired,
Review comment:
same comment
##########
File path:
superset-frontend/src/explore/components/controls/MetricControl/AdhocMetricEditPopover/index.jsx
##########
@@ -50,7 +50,7 @@ const propTypes = {
columns: PropTypes.arrayOf(columnType),
savedMetricsOptions: PropTypes.arrayOf(savedMetricType),
savedMetric: savedMetricType,
- datasourceType: PropTypes.string,
+ datasource: PropTypes.object.isRequired,
Review comment:
if datasource is marked as `isRequired` then you shouldn't need the
`datasource?.type` below and can just do `datasource.type`. But if datasource
isn't passed in sometimes, then you should remove the `isRequired` part
--
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]
