[GitHub] [superset] etr2460 commented on a change in pull request #15699: perf(dashboard): make loading datasets non-blocking

[GitBox <gi...@apache.org>]

etr2460 commented on a change in pull request #15699:
URL: https://github.com/apache/superset/pull/15699#discussion_r670438909



##########
File path: superset-frontend/src/common/hooks/apiResources/dashboards.ts
##########
@@ -33,10 +34,10 @@ export const useDashboard = (idOrSlug: string | number) =>
 
 // gets the chart definitions for a dashboard
 export const useDashboardCharts = (idOrSlug: string | number) =>
-  useApiV1Resource(`/api/v1/dashboard/${idOrSlug}/charts`);
+  useApiV1Resource<Chart[]>(`/api/v1/dashboard/${idOrSlug}/charts`);
 
 // gets the datasets for a dashboard
 // important: this endpoint only returns the fields in the dataset
 // that are necessary for rendering the given dashboard
 export const useDashboardDatasets = (idOrSlug: string | number) =>
-  useApiV1Resource(`/api/v1/dashboard/${idOrSlug}/datasets`);
+  useApiV1Resource<Datasource[]>(`/api/v1/dashboard/${idOrSlug}/datasets`);

Review comment:
       nit: perhaps we should rename the Datasource type to Dataset? Or at least alias it for now?

##########
File path: superset-frontend/src/explore/components/Control.tsx
##########
@@ -43,53 +43,38 @@ export type ControlProps = {
   renderTrigger?: boolean;
 };
 
-export default class Control extends React.PureComponent<
-  ControlProps,
-  { hovered: boolean }
-> {
-  onMouseEnter: () => void;
+export default function Control(props: ControlProps) {
+  const {
+    actions: { setControlValue },
+    name,
+    type,
+    hidden,
+  } = props;
 
-  onMouseLeave: () => void;
+  const [hovered, setHovered] = useState(false);
+  const onChange = useCallback(
+    (value: any, errors: any[]) => setControlValue(name, value, errors),
+    [name, setControlValue],
+  );
 
-  constructor(props: ControlProps) {
-    super(props);
-    this.state = { hovered: false };
-    this.onChange = this.onChange.bind(this);
-    this.onMouseEnter = this.setHover.bind(this, true);
-    this.onMouseLeave = this.setHover.bind(this, false);
-  }
-
-  onChange(value: any, errors: any[]) {
-    this.props.actions.setControlValue(this.props.name, value, errors);
-  }
+  if (!type) return null;
 
-  setHover(hovered: boolean) {
-    this.setState({ hovered });
+  const ControlComponent = typeof type === 'string' ? controlMap[type] : type;
+  if (!ControlComponent) {
+    return <>Unknown controlType: {type}</>;

Review comment:
       why do we need a fragment here? Couldn't this be:
   ```suggestion
       return `Unknown controlType: ${type}`;
   ```

##########
File path: superset-frontend/src/types/Dashboard.ts
##########
@@ -19,7 +19,7 @@
 import Owner from './Owner';
 import Role from './Role';
 
-type Dashboard = {
+export interface Dashboard {

Review comment:
       fwiw i think there's still a bunch of discrepancies across Superset for this. I don't really care one way or another (consistency is better in my mind) but noting that even after this change there will still be a bunch of inconsistencies




-- 
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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org