[GitHub] [superset] geido commented on a diff in pull request #20728: feature(dashboard): Drill to detail modal

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

geido commented on code in PR #20728:
URL: https://github.com/apache/superset/pull/20728#discussion_r925655214


##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, {
+  useState,
+  useEffect,
+  useMemo,
+  useCallback,
+  useRef,
+} from 'react';
+import {
+  css,
+  Datasource,
+  ensureIsArray,
+  GenericDataType,
+  QueryObjectFilterClause,
+  styled,
+  t,
+  useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+  margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+  datasource,
+  initialFilters,
+}: {
+  datasource: Datasource;
+  initialFilters?: QueryObjectFilterClause[];
+}) {
+  const theme = useTheme();
+  const pageResponses = useRef({});
+  const [results, setResults] = useState<{
+    total: number;
+    dataPage: Record<string, any>[];
+  } | null>();
+
+  const [colnames, setColnames] = useState<string[]>([]);
+  const [coltypes, setColtypes] = useState<GenericDataType[]>([]);
+  const [isLoading, setIsLoading] = useState<boolean>(false);
+  const [responseError, setResponseError] = useState<string>('');
+  const [filters, setFilters] = useState(initialFilters || []);
+  const [page, setPage] = useState(0);

Review Comment:
   I think page should start from 1



##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, {
+  useState,
+  useEffect,
+  useMemo,
+  useCallback,
+  useRef,
+} from 'react';
+import {
+  css,
+  Datasource,
+  ensureIsArray,
+  GenericDataType,
+  QueryObjectFilterClause,
+  styled,
+  t,
+  useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+  margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+  datasource,
+  initialFilters,
+}: {
+  datasource: Datasource;
+  initialFilters?: QueryObjectFilterClause[];
+}) {
+  const theme = useTheme();
+  const pageResponses = useRef({});
+  const [results, setResults] = useState<{
+    total: number;
+    dataPage: Record<string, any>[];
+  } | null>();
+

Review Comment:
   ```suggestion
   ```



##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, {
+  useState,
+  useEffect,
+  useMemo,
+  useCallback,
+  useRef,
+} from 'react';
+import {
+  css,
+  Datasource,
+  ensureIsArray,
+  GenericDataType,
+  QueryObjectFilterClause,
+  styled,
+  t,
+  useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`

Review Comment:
   Since this is only used once I believe it makes sense to define its styles inline using `css` from Emotion. Have a look at this https://github.com/apache/superset/wiki/Emotion-Styling-Guidelines-and-Best-Practices



##########
superset-frontend/src/dashboard/components/DatasourceResultsPane/index.tsx:
##########
@@ -0,0 +1,177 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import React, {
+  useState,
+  useEffect,
+  useMemo,
+  useCallback,
+  useRef,
+} from 'react';
+import {
+  css,
+  Datasource,
+  ensureIsArray,
+  GenericDataType,
+  QueryObjectFilterClause,
+  styled,
+  t,
+  useTheme,
+} from '@superset-ui/core';
+import Loading from 'src/components/Loading';
+import { EmptyStateMedium } from 'src/components/EmptyState';
+import TableView, { EmptyWrapperType } from 'src/components/TableView';
+import { useTableColumns } from 'src/explore/components/DataTableControl';
+import { getDatasetSamples } from 'src/components/Chart/chartAction';
+import DatasourceFilterBar from './DatasourceFilterBar';
+
+const Error = styled.pre`
+  margin-top: ${({ theme }) => `${theme.gridUnit * 4}px`};
+`;
+
+const PAGE_SIZE = 50;
+
+export default function DatasourceResultsPane({
+  datasource,
+  initialFilters,
+}: {
+  datasource: Datasource;
+  initialFilters?: QueryObjectFilterClause[];
+}) {
+  const theme = useTheme();
+  const pageResponses = useRef({});
+  const [results, setResults] = useState<{
+    total: number;
+    dataPage: Record<string, any>[];
+  } | null>();
+
+  const [colnames, setColnames] = useState<string[]>([]);
+  const [coltypes, setColtypes] = useState<GenericDataType[]>([]);
+  const [isLoading, setIsLoading] = useState<boolean>(false);
+  const [responseError, setResponseError] = useState<string>('');
+  const [filters, setFilters] = useState(initialFilters || []);
+  const [page, setPage] = useState(0);
+  const [sortBy, setSortBy] = useState([]);
+  const datasourceId = useMemo(
+    () => `${datasource.id}__${datasource.type}`,
+    [datasource],
+  );
+
+  useEffect(() => {
+    pageResponses.current = {};
+  }, [datasource, filters, sortBy]);
+
+  useEffect(() => {
+    const getPageData = async () => {
+      try {
+        setIsLoading(true);
+        let pageResponse = pageResponses.current[page];
+        if (!pageResponse) {

Review Comment:
   Should we consider the ability to refresh the cache? Users tend to work on different tabs and might expect to see updated data without having to hard refresh



-- 
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