geido commented on code in PR #28370:
URL: https://github.com/apache/superset/pull/28370#discussion_r1603264200
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -36,73 +38,102 @@ import { getUrlParam } from 'src/utils/urlUtils';
import { isCurrentUserBot } from 'src/utils/isBot';
import { ChartSource } from 'src/types/ChartSource';
import { ResourceStatus } from 'src/hooks/apiResources/apiResources';
+import { Dispatch } from 'redux';
import ChartRenderer from './ChartRenderer';
import { ChartErrorMessage } from './ChartErrorMessage';
import { getChartRequiredFieldsMissingMessage } from
'../../utils/getChartRequiredFieldsMissingMessage';
-const propTypes = {
- annotationData: PropTypes.object,
- actions: PropTypes.object,
- chartId: PropTypes.number.isRequired,
- datasource: PropTypes.object,
- // current chart is included by dashboard
- dashboardId: PropTypes.number,
- // original selected values for FilterBox viz
- // so that FilterBox can pre-populate selected values
- // only affect UI control
- initialValues: PropTypes.object,
- // formData contains chart's own filter parameter
- // and merged with extra filter that current dashboard applying
- formData: PropTypes.object.isRequired,
- labelColors: PropTypes.object,
- sharedLabelColors: PropTypes.object,
- width: PropTypes.number,
- height: PropTypes.number,
- setControlValue: PropTypes.func,
- timeout: PropTypes.number,
- vizType: PropTypes.string.isRequired,
- triggerRender: PropTypes.bool,
- force: PropTypes.bool,
- isFiltersInitialized: PropTypes.bool,
- // state
- chartAlert: PropTypes.string,
- chartStatus: PropTypes.string,
- chartStackTrace: PropTypes.string,
- queriesResponse: PropTypes.arrayOf(PropTypes.object),
- triggerQuery: PropTypes.bool,
- chartIsStale: PropTypes.bool,
- errorMessage: PropTypes.node,
- // dashboard callbacks
- addFilter: PropTypes.func,
- onQuery: PropTypes.func,
- onFilterMenuOpen: PropTypes.func,
- onFilterMenuClose: PropTypes.func,
- ownState: PropTypes.object,
- postTransformProps: PropTypes.func,
- datasetsStatus: PropTypes.oneOf(['loading', 'error', 'complete']),
- isInView: PropTypes.bool,
- emitCrossFilters: PropTypes.bool,
+export interface ChartProps {
+ annotationData?: Object;
+ actions: Actions;
+ chartId: string;
+ datasource?: {
+ database?: {
+ name: string;
+ };
+ };
+ dashboardId?: number;
+ initialValues?: object;
+ formData: QueryFormData;
+ labelColors?: object;
+ sharedLabelColors?: object;
+ width: number;
+ height: number;
+ setControlValue: Function;
+ timeout?: number;
+ vizType: string;
+ triggerRender?: boolean;
+ force?: boolean;
+ isFiltersInitialized?: boolean;
+ chartAlert?: string;
+ chartStatus?: string;
+ chartStackTrace?: string;
+ queriesResponse?: QueryResponse[];
+ triggerQuery?: boolean;
+ chartIsStale?: boolean;
+ errorMessage?: React.ReactNode;
+ addFilter?: Function;
+ onQuery?: MouseEventHandler<HTMLSpanElement>;
+ onFilterMenuOpen?: Function;
+ onFilterMenuClose?: Function;
+ ownState?: any;
Review Comment:
Can we add a better type for this?
##########
superset-frontend/src/components/Chart/ChartErrorMessage.tsx:
##########
@@ -21,10 +21,16 @@ import React from 'react';
import { SupersetError } from '@superset-ui/core';
import { useChartOwnerNames } from 'src/hooks/apiResources';
import ErrorMessageWithStackTrace from
'src/components/ErrorMessage/ErrorMessageWithStackTrace';
+import { ChartSource } from 'src/types/ChartSource';
-interface Props {
+export interface Props {
chartId: string;
error?: SupersetError;
+ subtitle: JSX.Element;
+ copyText: string;
+ link?: string;
+ source: ChartSource;
+ stackTrace?: string;
Review Comment:
I believe this could be an extension of
`superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts`
`ClientErrorObject`
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -36,73 +38,102 @@ import { getUrlParam } from 'src/utils/urlUtils';
import { isCurrentUserBot } from 'src/utils/isBot';
import { ChartSource } from 'src/types/ChartSource';
import { ResourceStatus } from 'src/hooks/apiResources/apiResources';
+import { Dispatch } from 'redux';
import ChartRenderer from './ChartRenderer';
import { ChartErrorMessage } from './ChartErrorMessage';
import { getChartRequiredFieldsMissingMessage } from
'../../utils/getChartRequiredFieldsMissingMessage';
-const propTypes = {
- annotationData: PropTypes.object,
- actions: PropTypes.object,
- chartId: PropTypes.number.isRequired,
- datasource: PropTypes.object,
- // current chart is included by dashboard
- dashboardId: PropTypes.number,
- // original selected values for FilterBox viz
- // so that FilterBox can pre-populate selected values
- // only affect UI control
- initialValues: PropTypes.object,
- // formData contains chart's own filter parameter
- // and merged with extra filter that current dashboard applying
- formData: PropTypes.object.isRequired,
- labelColors: PropTypes.object,
- sharedLabelColors: PropTypes.object,
- width: PropTypes.number,
- height: PropTypes.number,
- setControlValue: PropTypes.func,
- timeout: PropTypes.number,
- vizType: PropTypes.string.isRequired,
- triggerRender: PropTypes.bool,
- force: PropTypes.bool,
- isFiltersInitialized: PropTypes.bool,
- // state
- chartAlert: PropTypes.string,
- chartStatus: PropTypes.string,
- chartStackTrace: PropTypes.string,
- queriesResponse: PropTypes.arrayOf(PropTypes.object),
- triggerQuery: PropTypes.bool,
- chartIsStale: PropTypes.bool,
- errorMessage: PropTypes.node,
- // dashboard callbacks
- addFilter: PropTypes.func,
- onQuery: PropTypes.func,
- onFilterMenuOpen: PropTypes.func,
- onFilterMenuClose: PropTypes.func,
- ownState: PropTypes.object,
- postTransformProps: PropTypes.func,
- datasetsStatus: PropTypes.oneOf(['loading', 'error', 'complete']),
- isInView: PropTypes.bool,
- emitCrossFilters: PropTypes.bool,
+export interface ChartProps {
+ annotationData?: Object;
+ actions: Actions;
+ chartId: string;
+ datasource?: {
+ database?: {
+ name: string;
+ };
+ };
+ dashboardId?: number;
+ initialValues?: object;
+ formData: QueryFormData;
+ labelColors?: object;
+ sharedLabelColors?: object;
+ width: number;
+ height: number;
+ setControlValue: Function;
+ timeout?: number;
+ vizType: string;
+ triggerRender?: boolean;
+ force?: boolean;
+ isFiltersInitialized?: boolean;
+ chartAlert?: string;
+ chartStatus?: string;
+ chartStackTrace?: string;
+ queriesResponse?: QueryResponse[];
+ triggerQuery?: boolean;
+ chartIsStale?: boolean;
+ errorMessage?: React.ReactNode;
+ addFilter?: Function;
+ onQuery?: MouseEventHandler<HTMLSpanElement>;
+ onFilterMenuOpen?: Function;
+ onFilterMenuClose?: Function;
+ ownState?: any;
+ postTransformProps?: Function;
+ datasetsStatus?: 'loading' | 'error' | 'complete';
+ isInView?: boolean;
+ emitCrossFilters?: boolean;
+}
+
+export type QueryResponse = {
Review Comment:
This assumes that the response will always be an error. I think we already
have a correct type for this. We either need to adjust that to account for the
error or create one specifically for when get an error:
Instead of this QueryResponse see:`
superset-frontend/packages/superset-ui-core/src/query/types/QueryResponse.ts`
`ChartDataResponse`
For the error maybe
`superset-frontend/packages/superset-ui-core/src/query/getClientErrorObject.ts`
`ClientErrorObject`
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -201,7 +238,7 @@ class Chart extends React.PureComponent {
});
}
- renderErrorMessage(queryResponse) {
+ renderErrorMessage(QueryResponse: QueryResponse) {
Review Comment:
```suggestion
renderErrorMessage(queryResponse: QueryResponse) {
```
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -214,7 +209,7 @@ class Chart extends React.PureComponent<ChartProps, {}> {
});
}
- renderErrorMessage(queryResponse: queryResponse) {
+ renderErrorMessage(QueryResponse: QueryResponse) {
Review Comment:
Does not look like this was resolved
##########
superset-frontend/src/components/Chart/Chart.tsx:
##########
@@ -150,9 +153,16 @@ const MonospaceDiv = styled.div`
overflow-x: auto;
white-space: pre-wrap;
`;
+class Chart extends React.PureComponent<ChartProps, {}> {
+ static defaultProps = defaultProps;
+
+ renderStartTime: any;
Review Comment:
Let's proceed with the migration and if we need to fix that we can do it in
a separate PR
--
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]
