You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2022/11/10 21:58:48 UTC
[GitHub] [superset] lyndsiWilliams commented on a diff in pull request #21948: feat: Integrate ant d table component into DatasetPanel
lyndsiWilliams commented on code in PR #21948:
URL: https://github.com/apache/superset/pull/21948#discussion_r1019638395
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -0,0 +1,237 @@
+/**
+ * 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 from 'react';
+import { supersetTheme, t, styled } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Table, { ColumnsType, TableSize } from 'src/components/Table';
+import { alphabeticalSort } from 'src/components/Table/sorters';
+// @ts-ignore
+import LOADING_GIF from 'src/assets/images/loading.gif';
+import { ITableColumn } from './types';
+import MessageContent from './MessageContent';
+
+/**
+ * Enum defining CSS position options
+ */
+enum EPosition {
+ ABSOLUTE = 'absolute',
+ RELATIVE = 'relative',
+}
+
+/**
+ * Interface for StyledHeader
+ */
+interface StyledHeaderProps {
+ /**
+ * Determine the CSS positioning type
+ * Vertical centering of loader, No columns screen, and select table screen
+ * gets offset when the header position is relative and needs to be absolute, but table
+ * needs this positioned relative to render correctly
+ */
+ position: EPosition;
+}
+
+const LOADER_WIDTH = 200;
+const SPINNER_WIDTH = 120;
+const HALF = 0.5;
+const MARGIN_MULTIPLIER = 3;
+
+const StyledHeader = styled.div<StyledHeaderProps>`
+ position: ${(props: StyledHeaderProps) => props.position};
+ margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+ margin-top: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ font-size: ${({ theme }) => theme.gridUnit * 6}px;
+ font-weight: ${({ theme }) => theme.typography.weights.medium};
+ padding-bottom: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+
+ white-space: nowrap;
+ overflow: hidden;
+ text-overflow: ellipsis;
+
+ .anticon:first-of-type {
+ margin-right: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ }
+
+ .anticon:nth-of-type(2) {
+ margin-left: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ }
+`;
+
+const StyledTitle = styled.div`
+ margin-left: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+ margin-bottom: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+`;
+
+const LoaderContainer = styled.div`
+ padding: ${({ theme }) => theme.gridUnit * 8}px
+ ${({ theme }) => theme.gridUnit * 6}px;
+
+ display: flex;
+ align-items: center;
+ justify-content: center;
+ height: 100%;
+`;
+
+const StyledLoader = styled.div`
+ max-width: 50%;
+ width: ${LOADER_WIDTH}px;
+
+ img {
+ width: ${SPINNER_WIDTH}px;
+ margin-left: ${(LOADER_WIDTH - SPINNER_WIDTH) * HALF}px;
+ }
+
+ div {
+ width: 100%;
+ margin-top: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+ text-align: center;
+ font-weight: 400;
+ font-size: 16px;
Review Comment:
```suggestion
font-weight: ${({ theme }) => theme.typography.weights.normal};
font-size: ${({ theme }) => theme.typography.sizes.l}px;
```
These should use the superset theme.
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/types.ts:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.
+ */
+
+/**
+ * Interface for table columns dataset
+ */
+export interface ITableColumn {
+ /**
+ * Name of the column
+ */
+ name: string;
+ /**
+ * Datatype of the column
+ */
+ type: string;
+}
+
+/**
+ * Checks if a given item matches the ITableColumn interface
+ * @param item Object to check if it matches the ITableColumn interface
+ * @returns boolean true if matches interface
+ */
+export const isITableColumn = (item: any): boolean => {
+ let match = true;
+ const BASE_ERROR =
+ 'The object provided to isITableColumn does match the interface.';
+ if (typeof item?.name !== 'string') {
+ match = false;
+ // eslint-disable-next-line no-console
+ console.error(
+ `${BASE_ERROR} The property 'name' is required and must be a string`,
+ );
+ }
+ if (match && typeof item?.type !== 'string') {
+ match = false;
+ // eslint-disable-next-line no-console
+ console.error(
+ `${BASE_ERROR} The property 'type' is required and must be a string`,
+ );
+ }
+ return match;
+};
+
+export interface IDatabaseTable {
+ name: string;
+ columns: ITableColumn[];
+}
+
+/**
+ * Checks if a given item matches the isIDatabsetTable interface
+ * @param item Object to check if it matches the isIDatabsetTable interface
+ * @returns boolean true if matches interface
+ */
+export const isIDatabaseTable = (item: any): boolean => {
Review Comment:
Can this `any` be described?
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/types.ts:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.
+ */
+
+/**
+ * Interface for table columns dataset
+ */
+export interface ITableColumn {
+ /**
+ * Name of the column
+ */
+ name: string;
+ /**
+ * Datatype of the column
+ */
+ type: string;
+}
+
+/**
+ * Checks if a given item matches the ITableColumn interface
+ * @param item Object to check if it matches the ITableColumn interface
+ * @returns boolean true if matches interface
+ */
+export const isITableColumn = (item: any): boolean => {
Review Comment:
Can this `any` be described?
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.tsx:
##########
@@ -0,0 +1,237 @@
+/**
+ * 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 from 'react';
+import { supersetTheme, t, styled } from '@superset-ui/core';
+import Icons from 'src/components/Icons';
+import Table, { ColumnsType, TableSize } from 'src/components/Table';
+import { alphabeticalSort } from 'src/components/Table/sorters';
+// @ts-ignore
+import LOADING_GIF from 'src/assets/images/loading.gif';
+import { ITableColumn } from './types';
+import MessageContent from './MessageContent';
+
+/**
+ * Enum defining CSS position options
+ */
+enum EPosition {
+ ABSOLUTE = 'absolute',
+ RELATIVE = 'relative',
+}
+
+/**
+ * Interface for StyledHeader
+ */
+interface StyledHeaderProps {
+ /**
+ * Determine the CSS positioning type
+ * Vertical centering of loader, No columns screen, and select table screen
+ * gets offset when the header position is relative and needs to be absolute, but table
+ * needs this positioned relative to render correctly
+ */
+ position: EPosition;
+}
+
+const LOADER_WIDTH = 200;
+const SPINNER_WIDTH = 120;
+const HALF = 0.5;
+const MARGIN_MULTIPLIER = 3;
+
+const StyledHeader = styled.div<StyledHeaderProps>`
+ position: ${(props: StyledHeaderProps) => props.position};
+ margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+ margin-top: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ font-size: ${({ theme }) => theme.gridUnit * 6}px;
+ font-weight: ${({ theme }) => theme.typography.weights.medium};
+ padding-bottom: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+
+ white-space: nowrap;
+ overflow: hidden;
+ text-overflow: ellipsis;
+
+ .anticon:first-of-type {
+ margin-right: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ }
+
+ .anticon:nth-of-type(2) {
+ margin-left: ${({ theme }) => theme.gridUnit * (MARGIN_MULTIPLIER + 1)}px;
+ }
+`;
+
+const StyledTitle = styled.div`
+ margin-left: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+ margin-bottom: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+ font-weight: ${({ theme }) => theme.typography.weights.bold};
+`;
+
+const LoaderContainer = styled.div`
+ padding: ${({ theme }) => theme.gridUnit * 8}px
+ ${({ theme }) => theme.gridUnit * 6}px;
+
+ display: flex;
+ align-items: center;
+ justify-content: center;
+ height: 100%;
+`;
+
+const StyledLoader = styled.div`
+ max-width: 50%;
+ width: ${LOADER_WIDTH}px;
+
+ img {
+ width: ${SPINNER_WIDTH}px;
+ margin-left: ${(LOADER_WIDTH - SPINNER_WIDTH) * HALF}px;
+ }
+
+ div {
+ width: 100%;
+ margin-top: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+ text-align: center;
+ font-weight: 400;
+ font-size: 16px;
+ color: ${({ theme }) => theme.colors.grayscale.light1};
+ }
+`;
+
+const TableContainer = styled.div`
+ position: relative;
+ margin: ${({ theme }) => theme.gridUnit * MARGIN_MULTIPLIER}px;
+ overflow: scroll;
+ height: calc(100% - ${({ theme }) => theme.gridUnit * 36}px);
+`;
+
+const StyledTable = styled(Table)`
+ position: absolute;
+ left: 0;
+ top: 0;
+ bottom: 0;
+ right: 0;
+`;
+
+export const REFRESHING = t('Refreshing columns');
+export const COLUMN_TITLE = t('Table columns');
+export const ALT_LOADING = t('Loading');
+
+const pageSizeOptions = ['5', '10', '15', '25'];
+
+// Define the columns for Table instance
+export const tableColumnDefinition: ColumnsType<ITableColumn> = [
+ {
+ title: 'Column Name',
+ dataIndex: 'name',
+ key: 'name',
+ sorter: (a: ITableColumn, b: ITableColumn) =>
+ alphabeticalSort('name', a, b),
+ },
+ {
+ title: 'Datatype',
+ dataIndex: 'type',
+ key: 'type',
+ width: '100px',
Review Comment:
```suggestion
width: ${({ theme }) => theme.gridUnit * 25}px,
```
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/types.ts:
##########
@@ -0,0 +1,92 @@
+/**
+ * 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.
+ */
+
+/**
+ * Interface for table columns dataset
+ */
+export interface ITableColumn {
+ /**
+ * Name of the column
+ */
+ name: string;
+ /**
+ * Datatype of the column
+ */
+ type: string;
+}
+
+/**
+ * Checks if a given item matches the ITableColumn interface
+ * @param item Object to check if it matches the ITableColumn interface
+ * @returns boolean true if matches interface
+ */
+export const isITableColumn = (item: any): boolean => {
+ let match = true;
+ const BASE_ERROR =
+ 'The object provided to isITableColumn does match the interface.';
+ if (typeof item?.name !== 'string') {
+ match = false;
+ // eslint-disable-next-line no-console
+ console.error(
+ `${BASE_ERROR} The property 'name' is required and must be a string`,
+ );
+ }
+ if (match && typeof item?.type !== 'string') {
+ match = false;
+ // eslint-disable-next-line no-console
+ console.error(
+ `${BASE_ERROR} The property 'type' is required and must be a string`,
+ );
+ }
+ return match;
+};
+
+export interface IDatabaseTable {
+ name: string;
+ columns: ITableColumn[];
+}
+
+/**
+ * Checks if a given item matches the isIDatabsetTable interface
+ * @param item Object to check if it matches the isIDatabsetTable interface
+ * @returns boolean true if matches interface
+ */
+export const isIDatabaseTable = (item: any): boolean => {
+ let match = true;
+ if (typeof item?.name !== 'string') {
+ match = false;
+ }
+ if (match && !Array.isArray(item.columns)) {
+ match = false;
+ }
+ if (match && item.columns.length > 0) {
+ const invalid = item.columns.some((column: any, index: number) => {
Review Comment:
Can this `any` be described?
##########
superset-frontend/src/views/CRUD/data/dataset/AddDataset/DatasetPanel/DatasetPanel.test.tsx:
##########
@@ -18,24 +18,117 @@
*/
import React from 'react';
import { render, screen } from 'spec/helpers/testing-library';
-import DatasetPanel from 'src/views/CRUD/data/dataset/AddDataset/DatasetPanel';
+import DatasetPanel, {
+ REFRESHING,
+ ALT_LOADING,
+ tableColumnDefinition,
+ COLUMN_TITLE,
+} from './DatasetPanel';
+import { exampleColumns } from './fixtures';
+import {
+ SELECT_MESSAGE,
+ CREATE_MESSAGE,
+ VIEW_DATASET_MESSAGE,
+ SELECT_TABLE_TITLE,
+ NO_COLUMNS_TITLE,
+ NO_COLUMNS_DESCRIPTION,
+ ERROR_TITLE,
+ ERROR_DESCRIPTION,
+} from './MessageContent';
describe('DatasetPanel', () => {
it('renders a blank state DatasetPanel', () => {
- render(<DatasetPanel />);
+ render(<DatasetPanel hasError={false} columnList={[]} loading={false} />);
const blankDatasetImg = screen.getByRole('img', { name: /empty/i });
- const blankDatasetTitle = screen.getByText(/select dataset source/i);
- const blankDatasetDescription = screen.getByText(
- /datasets can be created from database tables or sql queries\. select a database table to the left or to open sql lab\. from there you can save the query as a dataset\./i,
- );
+ expect(blankDatasetImg).toBeVisible();
+ const blankDatasetTitle = screen.getByText(SELECT_TABLE_TITLE);
+ expect(blankDatasetTitle).toBeVisible();
+ const blankDatasetDescription1 = screen.getByText(SELECT_MESSAGE, {
+ exact: false,
+ });
+ expect(blankDatasetDescription1).toBeVisible();
+ const blankDatasetDescription2 = screen.getByText(VIEW_DATASET_MESSAGE, {
+ exact: false,
+ });
+ expect(blankDatasetDescription2).toBeVisible();
const sqlLabLink = screen.getByRole('button', {
- name: /create dataset from sql query/i,
+ name: CREATE_MESSAGE,
});
+ expect(sqlLabLink).toBeVisible();
+ });
+
+ it('renders a no columns screen', () => {
+ render(
+ <DatasetPanel
+ tableName="Name"
+ hasError={false}
+ columnList={[]}
+ loading={false}
+ />,
+ );
+
+ const blankDatasetImg = screen.getByRole('img', { name: /empty/i });
+ expect(blankDatasetImg).toBeVisible();
+ const noColumnsTitle = screen.getByText(NO_COLUMNS_TITLE);
+ expect(noColumnsTitle).toBeVisible();
+ const noColumnsDescription = screen.getByText(NO_COLUMNS_DESCRIPTION);
+ expect(noColumnsDescription).toBeVisible();
+ });
+
+ it('renders a loading screen', () => {
+ render(
+ <DatasetPanel
+ tableName="Name"
+ hasError={false}
+ columnList={[]}
+ loading
+ />,
+ );
+ const blankDatasetImg = screen.getByAltText(ALT_LOADING);
expect(blankDatasetImg).toBeVisible();
+ const blankDatasetTitle = screen.getByText(REFRESHING);
expect(blankDatasetTitle).toBeVisible();
- expect(blankDatasetDescription).toBeVisible();
- expect(sqlLabLink).toBeVisible();
+ });
+
+ it('renders an error screen', () => {
+ render(
+ <DatasetPanel
+ tableName="Name"
+ hasError
+ columnList={[]}
+ loading={false}
+ />,
+ );
+
+ const errorTitle = screen.getByText(ERROR_TITLE);
+ expect(errorTitle).toBeVisible();
+ const errorDescription = screen.getByText(ERROR_DESCRIPTION);
+ expect(errorDescription).toBeVisible();
+ });
+
+ it('renders a table with columns displayed', () => {
+ const tableName = 'example_name';
+ render(
+ <DatasetPanel
+ tableName={tableName}
+ hasError={false}
+ columnList={exampleColumns}
+ loading={false}
+ />,
+ );
+ expect(screen.getByText(tableName)).toBeVisible();
Review Comment:
```suggestion
it('renders a table with columns displayed', async () => {
const tableName = 'example_name';
render(
<DatasetPanel
tableName={tableName}
hasError={false}
columnList={exampleColumns}
loading={false}
/>,
);
expect(await screen.findByText(tableName)).toBeVisible();
```
This test has 5 act errors, if you change these lines and add this:
```
jest.mock(
'src/components/Icons/Icon',
() =>
({ fileName }: { fileName: string }) =>
<span role="img" aria-label={fileName.replace('_', '-')} />,
);
```
to the top of the test, it will get rid of all of the act errors.
--
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