From f915aac06fb10d69cc9fd02ace2910e95ca13408 Mon Sep 17 00:00:00 2001 From: EugeneTorap Date: Sun, 19 Feb 2023 11:26:59 +0300 Subject: [PATCH 01/10] Use antd table in FilterableTable --- superset-frontend/package.json | 2 - .../components/ResultSet/ResultSet.test.tsx | 10 +- .../src/SqlLab/components/ResultSet/index.tsx | 13 +- .../FilterableTable/FilterableTable.test.tsx | 22 -- .../src/components/FilterableTable/index.tsx | 353 +++--------------- .../src/types/react-table-config.d.ts | 1 - superset-frontend/webpack.config.js | 1 - 7 files changed, 66 insertions(+), 336 deletions(-) diff --git a/superset-frontend/package.json b/superset-frontend/package.json index 9d4755926eecb..17d54ca19d0e6 100644 --- a/superset-frontend/package.json +++ b/superset-frontend/package.json @@ -196,7 +196,6 @@ "react-table": "^7.8.0", "react-transition-group": "^2.5.3", "react-ultimate-pagination": "^1.3.0", - "react-virtualized": "9.19.1", "react-virtualized-auto-sizer": "^1.0.7", "react-window": "^1.8.8", "redux": "^4.2.1", @@ -267,7 +266,6 @@ "@types/react-select": "^3.0.19", "@types/react-table": "^7.0.19", "@types/react-ultimate-pagination": "^1.2.0", - "@types/react-virtualized": "^9.21.10", "@types/react-window": "^1.8.5", "@types/redux-localstorage": "^1.0.8", "@types/redux-mock-store": "^1.0.2", diff --git a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx index 7869a87ab1b61..14747c98145aa 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx @@ -224,14 +224,14 @@ describe('ResultSet', () => { }); test('renders if there is no limit in query.results but has queryLimit', async () => { - const { getByRole } = setup(mockedProps, mockStore(initialState)); - expect(getByRole('grid')).toBeInTheDocument(); + const { getByTestId } = setup(mockedProps, mockStore(initialState)); + expect(getByTestId('table-container')).toBeInTheDocument(); }); test('renders if there is a limit in query.results but not queryLimit', async () => { const props = { ...mockedProps, query: queryWithNoQueryLimit }; - const { getByRole } = setup(props, mockStore(initialState)); - expect(getByRole('grid')).toBeInTheDocument(); + const { getByTestId } = setup(props, mockStore(initialState)); + expect(getByTestId('table-container')).toBeInTheDocument(); }); test('Async queries - renders "Fetch data preview" button when data preview has no results', () => { @@ -256,7 +256,7 @@ describe('ResultSet', () => { test('Async queries - renders on the first call', () => { setup(asyncQueryProps, mockStore(initialState)); - expect(screen.getByRole('grid')).toBeVisible(); + expect(screen.getByTestId('table-container')).toBeVisible(); expect( screen.queryByRole('button', { name: /fetch data preview/i, diff --git a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx index f41d27650b7bc..6e4cadc39bc58 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx @@ -42,9 +42,7 @@ import { mountExploreUrl } from 'src/explore/exploreUtils'; import { postFormData } from 'src/explore/exploreUtils/formData'; import ProgressBar from 'src/components/ProgressBar'; import Loading from 'src/components/Loading'; -import FilterableTable, { - MAX_COLUMNS_FOR_TABLE, -} from 'src/components/FilterableTable'; +import FilterableTable from 'src/components/FilterableTable'; import CopyToClipboard from 'src/components/CopyToClipboard'; import { addDangerToast } from 'src/components/MessageToasts/actions'; import { prepareCopyToClipboardTabularData } from 'src/utils/common'; @@ -282,12 +280,7 @@ const ResultSet = ({ onChange={changeSearch} value={searchText} className="form-control input-sm" - disabled={columns.length > MAX_COLUMNS_FOR_TABLE} - placeholder={ - columns.length > MAX_COLUMNS_FOR_TABLE - ? t('Too many columns to filter') - : t('Filter results') - } + placeholder={t('Filter results')} /> )} @@ -486,7 +479,7 @@ const ResultSet = ({ // We need to calculate the height of this.renderRowsReturned() // if we want results panel to be proper height because the // FilterTable component needs an explicit height to render - // react-virtualized Table component + // antd Table component const rowsHeight = alertIsOpen ? height - alertContainerHeight : height - rowMessageHeight; diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx index 8cef35078dd06..685019b294ed2 100644 --- a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx +++ b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx @@ -20,7 +20,6 @@ import React from 'react'; import { ReactWrapper } from 'enzyme'; import { styledMount as mount } from 'spec/helpers/theming'; import FilterableTable, { - MAX_COLUMNS_FOR_TABLE, convertBigIntStrToNumber, } from 'src/components/FilterableTable'; import { render, screen } from 'spec/helpers/testing-library'; @@ -49,27 +48,6 @@ describe('FilterableTable', () => { expect(wrapper.find('.ReactVirtualized__Grid')).toExist(); expect(wrapper.find('.ReactVirtualized__Table__row')).toHaveLength(3); }); - it('renders a grid with 2 Grid rows for wide tables', () => { - const wideTableColumns = MAX_COLUMNS_FOR_TABLE + 1; - const wideTableMockedProps = { - orderedColumnKeys: Array.from( - Array(wideTableColumns), - (_, x) => `col_${x}`, - ), - data: [ - { - ...Array.from(Array(wideTableColumns)).map((val, x) => ({ - [`col_${x}`]: x, - })), - }, - ], - height: 500, - }; - const wideTableWrapper = mount( - , - ); - expect(wideTableWrapper.find('.ReactVirtualized__Grid')).toHaveLength(2); - }); it('filters on a string', () => { const props = { ...mockedProps, diff --git a/superset-frontend/src/components/FilterableTable/index.tsx b/superset-frontend/src/components/FilterableTable/index.tsx index e7e8f3ebf4c6b..5c8c804e71314 100644 --- a/superset-frontend/src/components/FilterableTable/index.tsx +++ b/superset-frontend/src/components/FilterableTable/index.tsx @@ -19,16 +19,6 @@ import JSONbig from 'json-bigint'; import React, { useEffect, useRef, useState } from 'react'; import { JSONTree } from 'react-json-tree'; -import { - AutoSizer, - Column, - Grid, - ScrollSync, - SortDirection, - SortDirectionType, - SortIndicator, - Table, -} from 'react-virtualized'; import { getMultipleTextDimensions, t, @@ -38,6 +28,7 @@ import { import Button from '../Button'; import CopyToClipboard from '../CopyToClipboard'; import ModalTrigger from '../ModalTrigger'; +import { Table, TableSize } from '../Table'; function safeJsonObjectParse( data: unknown, @@ -74,7 +65,6 @@ function renderBigIntStrToNumber(value: string | number) { return <>{convertBigIntStrToNumber(value)}; } -const GRID_POSITION_ADJUSTMENT = 4; const SCROLL_BAR_HEIGHT = 15; // This regex handles all possible number formats in javascript, including ints, floats, // exponential notation, NaN, and Infinity. @@ -188,9 +178,6 @@ const StyledFilterableTable = styled.div` `} `; -// when more than MAX_COLUMNS_FOR_TABLE are returned, switch from table to grid view -export const MAX_COLUMNS_FOR_TABLE = 50; - type CellDataType = string | number | null; type Datum = Record; @@ -232,13 +219,8 @@ const FilterableTable = ({ return newRow; }); - const [sortByState, setSortByState] = useState(undefined); - const [sortDirectionState, setSortDirectionState] = useState< - SortDirectionType | undefined - >(undefined); const [fitted, setFitted] = useState(false); const [list] = useState(() => formatTableData(data)); - const [displayedList, setDisplayedList] = useState(list); // columns that have complex type and were expanded into sub columns const [complexColumns] = useState>( @@ -278,8 +260,6 @@ const FilterableTable = ({ return jsonTreeTheme; }; - const getDatum = (list: Datum[], index: number) => list[index % list.length]; - const getWidthsForColumns = () => { const PADDING = 50; // accounts for cell padding and width of sorting icon const widthsByColumnKey = {}; @@ -377,6 +357,7 @@ const FilterableTable = ({ return values.some(v => v.includes(lowerCaseText)); }; + // @ts-ignore const rowClassName = ({ index }: { index: number }) => { let className = ''; if (striped) { @@ -385,31 +366,6 @@ const FilterableTable = ({ return className; }; - const sort = ({ - sortBy, - sortDirection, - }: { - sortBy: string; - sortDirection: SortDirectionType; - }) => { - const shouldClearSort = - sortDirectionState === SortDirection.DESC && sortByState === sortBy; - - if (shouldClearSort) { - setSortByState(undefined); - setSortDirectionState(undefined); - setDisplayedList([...list]); - } else { - setSortByState(sortBy); - setSortDirectionState(sortDirection); - setDisplayedList( - [...list].sort( - sortResults(sortBy, sortDirection === SortDirection.DESC), - ), - ); - } - }; - const addJsonModal = ( node: React.ReactNode, jsonObject: Record | unknown[], @@ -444,201 +400,35 @@ const FilterableTable = ({ return value; }; - const sortResults = - (sortBy: string, descending: boolean) => (a: Datum, b: Datum) => { - const aValue = parseNumberFromString(a[sortBy]); - const bValue = parseNumberFromString(b[sortBy]); - - // equal items sort equally - if (aValue === bValue) { - return 0; - } - - // nulls sort after anything else - if (aValue === null) { - return 1; - } - if (bValue === null) { - return -1; - } + const sortResults = (key: string, a: Datum, b: Datum) => { + const aValue = parseNumberFromString(a[key]); + const bValue = parseNumberFromString(b[key]); - if (descending) { - return aValue < bValue ? 1 : -1; - } - return aValue < bValue ? -1 : 1; - }; - - const sortGrid = (label: string) => { - sort({ - sortBy: label, - sortDirection: - sortDirectionState === SortDirection.DESC || sortByState !== label - ? SortDirection.ASC - : SortDirection.DESC, - }); - }; - - const renderTableHeader = ({ - dataKey, - label, - sortBy, - sortDirection, - }: { - dataKey: string; - label: string; - sortBy: string; - sortDirection: SortDirectionType; - }) => { - const className = - expandedColumns.indexOf(label) > -1 - ? 'header-style-disabled' - : 'header-style'; - - return ( -
- {label} - {sortBy === dataKey && } -
- ); - }; - - const renderGridCellHeader = ({ - columnIndex, - key, - style, - }: { - columnIndex: number; - key: string; - style: React.CSSProperties; - }) => { - const label = orderedColumnKeys[columnIndex]; - const className = - expandedColumns.indexOf(label) > -1 - ? 'header-style-disabled' - : 'header-style'; - return ( -
sortGrid(label)} - > - {label} - {sortByState === label && ( - - )} -
- ); - }; - - const renderGridCell = ({ - columnIndex, - key, - rowIndex, - style, - }: { - columnIndex: number; - key: string; - rowIndex: number; - style: React.CSSProperties; - }) => { - const columnKey = orderedColumnKeys[columnIndex]; - const cellData = displayedList[rowIndex][columnKey]; - const cellText = getCellContent({ cellData, columnKey }); - const content = - cellData === null ? {cellText} : cellText; - const cellNode = ( -
-
{content}
-
- ); + // equal items sort equally + if (aValue === bValue) { + return 0; + } - const jsonObject = safeJsonObjectParse(cellData); - if (jsonObject) { - return addJsonModal(cellNode, jsonObject, cellData); + // nulls sort after anything else + if (aValue === null) { + return 1; + } + if (bValue === null) { + return -1; } - return cellNode; + + return aValue < bValue ? -1 : 1; }; - const renderGrid = () => { - // exclude the height of the horizontal scroll bar from the height of the table - // and the height of the table container if the content overflows - const totalTableHeight = - container.current && - totalTableWidth.current > container.current.clientWidth - ? height - SCROLL_BAR_HEIGHT - : height; - - const getColumnWidth = ({ index }: { index: number }) => - widthsForColumnsByKey[orderedColumnKeys[index]]; - - // fix height of filterable table - return ( - - - {({ onScroll, scrollLeft }) => ( - <> - - {({ width }) => ( -
- - -
- )} - - - )} - - + let filteredList = list; + // filter list + if (filterText) { + filteredList = filteredList.filter((row: Datum) => + hasMatch(filterText, row), ); - }; + } - const renderTableCell = ({ - cellData, - columnKey, - }: { - cellData: CellDataType; - columnKey: string; - }) => { + const renderTableCell = (cellData: CellDataType, columnKey: string) => { const cellNode = getCellContent({ cellData, columnKey }); const content = cellData === null ? {cellNode} : cellNode; @@ -649,68 +439,41 @@ const FilterableTable = ({ return content; }; - const renderTable = () => { - let sortedAndFilteredList = displayedList; - // filter list - if (filterText) { - sortedAndFilteredList = sortedAndFilteredList.filter((row: Datum) => - hasMatch(filterText, row), - ); - } - - // exclude the height of the horizontal scroll bar from the height of the table - // and the height of the table container if the content overflows - const totalTableHeight = - container.current && - totalTableWidth.current > container.current.clientWidth - ? height - SCROLL_BAR_HEIGHT - : height; - - const rowGetter = ({ index }: { index: number }) => - getDatum(sortedAndFilteredList, index); - return ( - - {fitted && ( - - {orderedColumnKeys.map(columnKey => ( - - renderTableCell({ cellData, columnKey }) - } - dataKey={columnKey} - disableSort={false} - headerRenderer={renderTableHeader} - width={widthsForColumnsByKey[columnKey]} - label={columnKey} - key={columnKey} - /> - ))} -
- )} - - ); - }; + // exclude the height of the horizontal scroll bar from the height of the table + // and the height of the table container if the content overflows + const totalTableHeight = + container.current && totalTableWidth.current > container.current.clientWidth + ? height - SCROLL_BAR_HEIGHT + : height; - if (orderedColumnKeys.length > MAX_COLUMNS_FOR_TABLE) { - return renderGrid(); - } - return renderTable(); + const columns = orderedColumnKeys.map(key => ({ + key, + title: key, + dataIndex: key, + width: widthsForColumnsByKey[key], + sorter: (a: Datum, b: Datum) => sortResults(key, a, b), + render: (text: CellDataType) => renderTableCell(text, key), + })); + + return ( + + {fitted && ( + + )} + + ); }; export default FilterableTable; diff --git a/superset-frontend/src/types/react-table-config.d.ts b/superset-frontend/src/types/react-table-config.d.ts index aa932c85997bb..7f769c36d6bf9 100644 --- a/superset-frontend/src/types/react-table-config.d.ts +++ b/superset-frontend/src/types/react-table-config.d.ts @@ -64,7 +64,6 @@ import { UseSortByOptions, UseSortByState, } from 'react-table'; -import { ColumnSizer } from 'react-virtualized'; declare module 'react-table' { type ColumnSize = 'xs' | 'sm' | 'md' | 'lg' | 'xl'; diff --git a/superset-frontend/webpack.config.js b/superset-frontend/webpack.config.js index f8428e4418fbb..cf61a352ed63a 100644 --- a/superset-frontend/webpack.config.js +++ b/superset-frontend/webpack.config.js @@ -250,7 +250,6 @@ const config = { 'react-hot-loader', 'react-select', 'react-sortable-hoc', - 'react-virtualized', 'react-table', 'react-ace', '@hot-loader.*', From c972f1c0292dcc63f008b142dbed66cbfa86b97d Mon Sep 17 00:00:00 2001 From: justin-park Date: Thu, 4 May 2023 10:18:44 -0700 Subject: [PATCH 02/10] update package-lock --- superset-frontend/package-lock.json | 66 +++++------------------------ 1 file changed, 10 insertions(+), 56 deletions(-) diff --git a/superset-frontend/package-lock.json b/superset-frontend/package-lock.json index cd0f4417d377e..0cc0a7be1cd12 100644 --- a/superset-frontend/package-lock.json +++ b/superset-frontend/package-lock.json @@ -131,7 +131,6 @@ "react-table": "^7.8.0", "react-transition-group": "^2.5.3", "react-ultimate-pagination": "^1.3.0", - "react-virtualized": "9.19.1", "react-virtualized-auto-sizer": "^1.0.7", "react-window": "^1.8.8", "redux": "^4.2.1", @@ -202,7 +201,6 @@ "@types/react-select": "^3.0.19", "@types/react-table": "^7.0.19", "@types/react-ultimate-pagination": "^1.2.0", - "@types/react-virtualized": "^9.21.10", "@types/react-window": "^1.8.5", "@types/redux-localstorage": "^1.0.8", "@types/redux-mock-store": "^1.0.2", @@ -21015,16 +21013,6 @@ "@types/react": "*" } }, - "node_modules/@types/react-virtualized": { - "version": "9.21.10", - "resolved": "https://registry.npmjs.org/@types/react-virtualized/-/react-virtualized-9.21.10.tgz", - "integrity": "sha512-f5Ti3A7gGdLkPPFNHTrvKblpsPNBiQoSorOEOD+JPx72g/Ng2lOt4MYfhvQFQNgyIrAro+Z643jbcKafsMW2ag==", - "dev": true, - "dependencies": { - "@types/prop-types": "*", - "@types/react": "*" - } - }, "node_modules/@types/react-window": { "version": "1.8.5", "resolved": "https://registry.npmjs.org/@types/react-window/-/react-window-1.8.5.tgz", @@ -24944,6 +24932,7 @@ "version": "6.26.0", "resolved": "https://registry.npmjs.org/babel-runtime/-/babel-runtime-6.26.0.tgz", "integrity": "sha1-llxwWGaOgrVde/4E/yM3vItWR/4=", + "dev": true, "dependencies": { "core-js": "^2.4.0", "regenerator-runtime": "^0.11.0" @@ -24953,12 +24942,14 @@ "version": "2.6.0", "resolved": "https://registry.npmjs.org/core-js/-/core-js-2.6.0.tgz", "integrity": "sha512-kLRC6ncVpuEW/1kwrOXYX6KQASCVtrh1gQr/UiaVgFlf9WE5Vp+lNe5+h3LuMr5PAucWnnEXwH0nQHRH/gpGtw==", - "deprecated": "core-js@<3.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Please, upgrade your dependencies to the actual version of core-js." + "deprecated": "core-js@<3.3 is no longer maintained and not recommended for usage due to the number of issues. Because of the V8 engine whims, feature detection in old core-js versions could cause a slowdown up to 100x even if nothing is polyfilled. Please, upgrade your dependencies to the actual version of core-js.", + "dev": true }, "node_modules/babel-runtime/node_modules/regenerator-runtime": { "version": "0.11.1", "resolved": "https://registry.npmjs.org/regenerator-runtime/-/regenerator-runtime-0.11.1.tgz", - "integrity": "sha512-MguG95oij0fC3QV3URf4V2SDYGJhJnJGqvIIgdECeODCT98wSWDAJ94SSuVpYQUoTcGUIL6L4yNB7j1DFFHSBg==" + "integrity": "sha512-MguG95oij0fC3QV3URf4V2SDYGJhJnJGqvIIgdECeODCT98wSWDAJ94SSuVpYQUoTcGUIL6L4yNB7j1DFFHSBg==", + "dev": true }, "node_modules/bail": { "version": "1.0.5", @@ -50854,23 +50845,6 @@ "react-dom": ">=16.13" } }, - "node_modules/react-virtualized": { - "version": "9.19.1", - "resolved": "https://registry.npmjs.org/react-virtualized/-/react-virtualized-9.19.1.tgz", - "integrity": "sha512-2l6uFicZKZ3x4rdnS0W+1TfyLmPO/+hfZKsCtoChoSmH5aEezGLpSuHc7oplekNIOaEwChfCk30zjx+Zw6B8YQ==", - "dependencies": { - "babel-runtime": "^6.26.0", - "classnames": "^2.2.3", - "dom-helpers": "^2.4.0 || ^3.0.0", - "loose-envify": "^1.3.0", - "prop-types": "^15.6.0", - "react-lifecycles-compat": "^3.0.4" - }, - "peerDependencies": { - "react": "^15.3.0 || ^16.0.0-alpha", - "react-dom": "^15.3.0 || ^16.0.0-alpha" - } - }, "node_modules/react-virtualized-auto-sizer": { "version": "1.0.7", "resolved": "https://registry.npmjs.org/react-virtualized-auto-sizer/-/react-virtualized-auto-sizer-1.0.7.tgz", @@ -78852,16 +78826,6 @@ "@types/react": "*" } }, - "@types/react-virtualized": { - "version": "9.21.10", - "resolved": "https://registry.npmjs.org/@types/react-virtualized/-/react-virtualized-9.21.10.tgz", - "integrity": "sha512-f5Ti3A7gGdLkPPFNHTrvKblpsPNBiQoSorOEOD+JPx72g/Ng2lOt4MYfhvQFQNgyIrAro+Z643jbcKafsMW2ag==", - "dev": true, - "requires": { - "@types/prop-types": "*", - "@types/react": "*" - } - }, "@types/react-window": { "version": "1.8.5", "resolved": "https://registry.npmjs.org/@types/react-window/-/react-window-1.8.5.tgz", @@ -81970,6 +81934,7 @@ "version": "6.26.0", "resolved": "https://registry.npmjs.org/babel-runtime/-/babel-runtime-6.26.0.tgz", "integrity": "sha1-llxwWGaOgrVde/4E/yM3vItWR/4=", + "dev": true, "requires": { "core-js": "^2.4.0", "regenerator-runtime": "^0.11.0" @@ -81978,12 +81943,14 @@ "core-js": { "version": "2.6.0", "resolved": "https://registry.npmjs.org/core-js/-/core-js-2.6.0.tgz", - "integrity": "sha512-kLRC6ncVpuEW/1kwrOXYX6KQASCVtrh1gQr/UiaVgFlf9WE5Vp+lNe5+h3LuMr5PAucWnnEXwH0nQHRH/gpGtw==" + "integrity": "sha512-kLRC6ncVpuEW/1kwrOXYX6KQASCVtrh1gQr/UiaVgFlf9WE5Vp+lNe5+h3LuMr5PAucWnnEXwH0nQHRH/gpGtw==", + "dev": true }, "regenerator-runtime": { "version": "0.11.1", "resolved": "https://registry.npmjs.org/regenerator-runtime/-/regenerator-runtime-0.11.1.tgz", - "integrity": "sha512-MguG95oij0fC3QV3URf4V2SDYGJhJnJGqvIIgdECeODCT98wSWDAJ94SSuVpYQUoTcGUIL6L4yNB7j1DFFHSBg==" + "integrity": "sha512-MguG95oij0fC3QV3URf4V2SDYGJhJnJGqvIIgdECeODCT98wSWDAJ94SSuVpYQUoTcGUIL6L4yNB7j1DFFHSBg==", + "dev": true } } }, @@ -101850,19 +101817,6 @@ "debounce": "^1.2.1" } }, - "react-virtualized": { - "version": "9.19.1", - "resolved": "https://registry.npmjs.org/react-virtualized/-/react-virtualized-9.19.1.tgz", - "integrity": "sha512-2l6uFicZKZ3x4rdnS0W+1TfyLmPO/+hfZKsCtoChoSmH5aEezGLpSuHc7oplekNIOaEwChfCk30zjx+Zw6B8YQ==", - "requires": { - "babel-runtime": "^6.26.0", - "classnames": "^2.2.3", - "dom-helpers": "^2.4.0 || ^3.0.0", - "loose-envify": "^1.3.0", - "prop-types": "^15.6.0", - "react-lifecycles-compat": "^3.0.4" - } - }, "react-virtualized-auto-sizer": { "version": "1.0.7", "resolved": "https://registry.npmjs.org/react-virtualized-auto-sizer/-/react-virtualized-auto-sizer-1.0.7.tgz", From 70139fbecf92d84dd736312523cf7c0bf08549a6 Mon Sep 17 00:00:00 2001 From: justin-park Date: Thu, 4 May 2023 10:19:10 -0700 Subject: [PATCH 03/10] replace to getByRole --- .../src/SqlLab/components/ResultSet/ResultSet.test.tsx | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx index 14747c98145aa..1d9fd58be5f8a 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx @@ -224,14 +224,14 @@ describe('ResultSet', () => { }); test('renders if there is no limit in query.results but has queryLimit', async () => { - const { getByTestId } = setup(mockedProps, mockStore(initialState)); - expect(getByTestId('table-container')).toBeInTheDocument(); + const { getByRole } = setup(mockedProps, mockStore(initialState)); + expect(getByRole('table')).toBeInTheDocument(); }); test('renders if there is a limit in query.results but not queryLimit', async () => { const props = { ...mockedProps, query: queryWithNoQueryLimit }; - const { getByTestId } = setup(props, mockStore(initialState)); - expect(getByTestId('table-container')).toBeInTheDocument(); + const { getByRole } = setup(props, mockStore(initialState)); + expect(getByRole('table')).toBeInTheDocument(); }); test('Async queries - renders "Fetch data preview" button when data preview has no results', () => { @@ -256,7 +256,7 @@ describe('ResultSet', () => { test('Async queries - renders on the first call', () => { setup(asyncQueryProps, mockStore(initialState)); - expect(screen.getByTestId('table-container')).toBeVisible(); + expect(screen.getByRole('table')).toBeVisible(); expect( screen.queryByRole('button', { name: /fetch data preview/i, From 91c67bb5c2509454d617bd0baf9f548e3a754982 Mon Sep 17 00:00:00 2001 From: justin-park Date: Thu, 4 May 2023 10:24:12 -0700 Subject: [PATCH 04/10] update comments --- superset-frontend/src/SqlLab/components/ResultSet/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx index 6e4cadc39bc58..b614a0efb3ed8 100644 --- a/superset-frontend/src/SqlLab/components/ResultSet/index.tsx +++ b/superset-frontend/src/SqlLab/components/ResultSet/index.tsx @@ -479,7 +479,7 @@ const ResultSet = ({ // We need to calculate the height of this.renderRowsReturned() // if we want results panel to be proper height because the // FilterTable component needs an explicit height to render - // antd Table component + // the Table component const rowsHeight = alertIsOpen ? height - alertContainerHeight : height - rowMessageHeight; From 51c2302738363881afb6fc892b0ee3df388cec07 Mon Sep 17 00:00:00 2001 From: justin-park Date: Thu, 4 May 2023 14:06:20 -0700 Subject: [PATCH 05/10] clean up addressed issues including debounced search --- .../src/components/FilterableTable/index.tsx | 133 +++++++----------- .../src/components/Table/index.tsx | 8 +- .../src/hooks/useDebounceValue.test.ts | 85 +++++++++++ .../src/hooks/useDebounceValue.ts | 36 +++++ 4 files changed, 182 insertions(+), 80 deletions(-) create mode 100644 superset-frontend/src/hooks/useDebounceValue.test.ts create mode 100644 superset-frontend/src/hooks/useDebounceValue.ts diff --git a/superset-frontend/src/components/FilterableTable/index.tsx b/superset-frontend/src/components/FilterableTable/index.tsx index 5c8c804e71314..4db5f0579c132 100644 --- a/superset-frontend/src/components/FilterableTable/index.tsx +++ b/superset-frontend/src/components/FilterableTable/index.tsx @@ -17,14 +17,16 @@ * under the License. */ import JSONbig from 'json-bigint'; -import React, { useEffect, useRef, useState } from 'react'; +import React, { useEffect, useRef, useState, useMemo } from 'react'; import { JSONTree } from 'react-json-tree'; import { getMultipleTextDimensions, t, styled, useTheme, + FAST_DEBOUNCE, } from '@superset-ui/core'; +import { useDebounceValue } from 'src/hooks/useDebounceValue'; import Button from '../Button'; import CopyToClipboard from '../CopyToClipboard'; import ModalTrigger from '../ModalTrigger'; @@ -74,28 +76,14 @@ const ONLY_NUMBER_REGEX = /^(NaN|-?((\d*\.\d+|\d+)([Ee][+-]?\d+)?|Infinity))$/; const StyledFilterableTable = styled.div` ${({ theme }) => ` height: 100%; - overflow-x: auto; + overflow: hidden; margin-top: ${theme.gridUnit * 2}px; - overflow-y: hidden; - .ReactVirtualized__Grid__innerScrollContainer { - border: 1px solid ${theme.colors.grayscale.light2}; - } - - .ReactVirtualized__Table__headerRow { + .ant-table-cell { font-weight: ${theme.typography.weights.bold}; - display: flex; - flex-direction: row; - align-items: center; - border: 1px solid ${theme.colors.grayscale.light2}; + background-color: ${theme.colors.grayscale.light5}; } - .ReactVirtualized__Table__row { - display: flex; - flex-direction: row; - } - - .ReactVirtualized__Table__headerTruncatedText, .grid-header-cell { display: inline-block; max-width: 100%; @@ -104,13 +92,10 @@ const StyledFilterableTable = styled.div` overflow: hidden; } - .ReactVirtualized__Table__headerColumn, - .ReactVirtualized__Table__rowColumn, - .grid-cell { + .ant-table-cell, + .virtual-table-cell { min-width: 0px; - border-right: 1px solid ${theme.colors.grayscale.light2}; align-self: center; - padding: ${theme.gridUnit * 3}px; font-size: ${theme.typography.sizes.s}px; } @@ -190,6 +175,7 @@ export interface FilterableTableProps { overscanColumnCount?: number; overscanRowCount?: number; rowHeight?: number; + // need antd 5.0 to support striped color pattern striped?: boolean; expandedColumns?: string[]; } @@ -199,11 +185,6 @@ const FilterableTable = ({ data, height, filterText = '', - headerHeight = 32, - overscanColumnCount = 10, - overscanRowCount = 10, - rowHeight = 32, - striped = true, expandedColumns = [], }: FilterableTableProps) => { const formatTableData = (data: Record[]): Datum[] => @@ -223,16 +204,41 @@ const FilterableTable = ({ const [list] = useState(() => formatTableData(data)); // columns that have complex type and were expanded into sub columns - const [complexColumns] = useState>( - orderedColumnKeys.reduce( - (obj, key) => ({ - ...obj, - [key]: expandedColumns.some(name => name.startsWith(`${key}.`)), - }), - {}, - ), + const complexColumns = useMemo>( + () => + orderedColumnKeys.reduce( + (obj, key) => ({ + ...obj, + [key]: expandedColumns.some(name => name.startsWith(`${key}.`)), + }), + {}, + ), + [expandedColumns, orderedColumnKeys], ); + const getCellContent = ({ + cellData, + columnKey, + }: { + cellData: CellDataType; + columnKey: string; + }) => { + if (cellData === null) { + return 'NULL'; + } + const content = String(cellData); + const firstCharacter = content.substring(0, 1); + let truncated; + if (firstCharacter === '[') { + truncated = '[…]'; + } else if (firstCharacter === '{') { + truncated = '{…}'; + } else { + truncated = ''; + } + return complexColumns[columnKey] ? truncated : content; + }; + const theme = useTheme(); const [jsonTreeTheme, setJsonTreeTheme] = useState>(); @@ -291,29 +297,6 @@ const FilterableTable = ({ return widthsByColumnKey; }; - const getCellContent = ({ - cellData, - columnKey, - }: { - cellData: CellDataType; - columnKey: string; - }) => { - if (cellData === null) { - return 'NULL'; - } - const content = String(cellData); - const firstCharacter = content.substring(0, 1); - let truncated; - if (firstCharacter === '[') { - truncated = '[…]'; - } else if (firstCharacter === '{') { - truncated = '{…}'; - } else { - truncated = ''; - } - return complexColumns[columnKey] ? truncated : content; - }; - const [widthsForColumnsByKey] = useState>(() => getWidthsForColumns(), ); @@ -357,16 +340,7 @@ const FilterableTable = ({ return values.some(v => v.includes(lowerCaseText)); }; - // @ts-ignore - const rowClassName = ({ index }: { index: number }) => { - let className = ''; - if (striped) { - className = index % 2 === 0 ? 'even-row' : 'odd-row'; - } - return className; - }; - - const addJsonModal = ( + const renderJsonModal = ( node: React.ReactNode, jsonObject: Record | unknown[], jsonString: CellDataType, @@ -420,13 +394,13 @@ const FilterableTable = ({ return aValue < bValue ? -1 : 1; }; - let filteredList = list; - // filter list - if (filterText) { - filteredList = filteredList.filter((row: Datum) => - hasMatch(filterText, row), - ); - } + const keyword = useDebounceValue(filterText, FAST_DEBOUNCE); + + const filteredList = useMemo( + () => + keyword ? list.filter((row: Datum) => hasMatch(keyword, row)) : list, + [list, keyword], + ); const renderTableCell = (cellData: CellDataType, columnKey: string) => { const cellNode = getCellContent({ cellData, columnKey }); @@ -434,7 +408,7 @@ const FilterableTable = ({ cellData === null ? {cellNode} : cellNode; const jsonObject = safeJsonObjectParse(cellData); if (jsonObject) { - return addJsonModal(cellNode, jsonObject, cellData); + return renderJsonModal(cellNode, jsonObject, cellData); } return content; }; @@ -463,13 +437,14 @@ const FilterableTable = ({ > {fitted && (
)} diff --git a/superset-frontend/src/components/Table/index.tsx b/superset-frontend/src/components/Table/index.tsx index 662908a7508b0..ff951b5bd93ad 100644 --- a/superset-frontend/src/components/Table/index.tsx +++ b/superset-frontend/src/components/Table/index.tsx @@ -21,7 +21,7 @@ import AntTable, { ColumnsType, TableProps as AntTableProps, } from 'antd/lib/table'; -import ConfigProvider from 'antd/lib/config-provider'; +import ConfigProvider, { ConfigProviderProps } from 'antd/lib/config-provider'; import { PaginationProps } from 'antd/lib/pagination'; import { t, useTheme, logging, styled } from '@superset-ui/core'; import Loading from 'src/components/Loading'; @@ -58,6 +58,10 @@ export interface TableProps { * Data that will populate the each row and map to the column key. */ data: RecordType[]; + /** + * Whether to show all table borders + */ + bordered?: boolean; /** * Table column definitions. */ @@ -224,6 +228,7 @@ export function Table( ) { const { data, + bordered, columns, selectedRows = defaultRowSelection, handleRowSelection, @@ -376,6 +381,7 @@ export function Table( onRow, theme, height: bodyHeight, + bordered, }; return ( diff --git a/superset-frontend/src/hooks/useDebounceValue.test.ts b/superset-frontend/src/hooks/useDebounceValue.test.ts new file mode 100644 index 0000000000000..ae0bdc2fcb316 --- /dev/null +++ b/superset-frontend/src/hooks/useDebounceValue.test.ts @@ -0,0 +1,85 @@ +/** + * 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 { act, renderHook } from '@testing-library/react-hooks'; +import { useDebounceValue } from './useDebounceValue'; + +describe('useDebounceValue', () => { + afterEach(() => { + jest.clearAllTimers(); + jest.useRealTimers(); + }); + + it('should return the initial value', () => { + const { result } = renderHook(() => useDebounceValue('hello')); + expect(result.current).toBe('hello'); + }); + + it('should update debounced value after delay', async () => { + jest.useFakeTimers(); + const { result, rerender } = renderHook( + ({ value, delay }) => useDebounceValue(value, delay), + { initialProps: { value: 'hello', delay: 1000 } }, + ); + + expect(result.current).toBe('hello'); + act(() => { + rerender({ value: 'world', delay: 1000 }); + jest.advanceTimersByTime(500); + }); + + expect(result.current).toBe('hello'); + + act(() => { + jest.advanceTimersByTime(1000); + }); + + expect(result.current).toBe('world'); + }); + + it('should cancel previous timeout when value changes', async () => { + jest.useFakeTimers(); + const { result, rerender } = renderHook( + ({ value, delay }) => useDebounceValue(value, delay), + { initialProps: { value: 'hello', delay: 1000 } }, + ); + + expect(result.current).toBe('hello'); + rerender({ value: 'world', delay: 1000 }); + + jest.advanceTimersByTime(500); + rerender({ value: 'foo', delay: 1000 }); + + jest.advanceTimersByTime(500); + expect(result.current).toBe('hello'); + }); + + it('should cancel the timeout when unmounting', async () => { + jest.useFakeTimers(); + const { result, unmount } = renderHook(() => + useDebounceValue('hello', 1000), + ); + + expect(result.current).toBe('hello'); + unmount(); + + jest.advanceTimersByTime(1000); + expect(clearTimeout).toHaveBeenCalled(); + }); +}); diff --git a/superset-frontend/src/hooks/useDebounceValue.ts b/superset-frontend/src/hooks/useDebounceValue.ts new file mode 100644 index 0000000000000..999394b0236b4 --- /dev/null +++ b/superset-frontend/src/hooks/useDebounceValue.ts @@ -0,0 +1,36 @@ +/** + * 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 { useState, useEffect } from 'react'; + +export function useDebounceValue(value: string, delay = 500) { + const [debouncedValue, setDebouncedValue] = useState(value); + + useEffect(() => { + const handler: NodeJS.Timeout = setTimeout(() => { + setDebouncedValue(value); + }, delay); + + // Cancel the timeout if value changes (also on delay change or unmount) + return () => { + clearTimeout(handler); + }; + }, [value, delay]); + + return debouncedValue; +} From 8cf23a48ab1c4ff974718f825739d8f88c384a22 Mon Sep 17 00:00:00 2001 From: justin-park Date: Thu, 4 May 2023 15:50:38 -0700 Subject: [PATCH 06/10] remove unused --- superset-frontend/src/components/Table/index.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/superset-frontend/src/components/Table/index.tsx b/superset-frontend/src/components/Table/index.tsx index ff951b5bd93ad..29a512093302c 100644 --- a/superset-frontend/src/components/Table/index.tsx +++ b/superset-frontend/src/components/Table/index.tsx @@ -21,7 +21,7 @@ import AntTable, { ColumnsType, TableProps as AntTableProps, } from 'antd/lib/table'; -import ConfigProvider, { ConfigProviderProps } from 'antd/lib/config-provider'; +import ConfigProvider from 'antd/lib/config-provider'; import { PaginationProps } from 'antd/lib/pagination'; import { t, useTheme, logging, styled } from '@superset-ui/core'; import Loading from 'src/components/Loading'; From 7e5aa703e587e8b85ab66c4423663ef6621f8fce Mon Sep 17 00:00:00 2001 From: justin-park Date: Fri, 5 May 2023 15:10:41 -0700 Subject: [PATCH 07/10] update FAST_DEBOUNCE reference --- superset-frontend/src/components/FilterableTable/index.tsx | 3 +-- superset-frontend/src/hooks/useDebounceValue.ts | 3 ++- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/superset-frontend/src/components/FilterableTable/index.tsx b/superset-frontend/src/components/FilterableTable/index.tsx index 4db5f0579c132..fad52a66bb8b4 100644 --- a/superset-frontend/src/components/FilterableTable/index.tsx +++ b/superset-frontend/src/components/FilterableTable/index.tsx @@ -24,7 +24,6 @@ import { t, styled, useTheme, - FAST_DEBOUNCE, } from '@superset-ui/core'; import { useDebounceValue } from 'src/hooks/useDebounceValue'; import Button from '../Button'; @@ -394,7 +393,7 @@ const FilterableTable = ({ return aValue < bValue ? -1 : 1; }; - const keyword = useDebounceValue(filterText, FAST_DEBOUNCE); + const keyword = useDebounceValue(filterText); const filteredList = useMemo( () => diff --git a/superset-frontend/src/hooks/useDebounceValue.ts b/superset-frontend/src/hooks/useDebounceValue.ts index 999394b0236b4..711b2dbd5a98c 100644 --- a/superset-frontend/src/hooks/useDebounceValue.ts +++ b/superset-frontend/src/hooks/useDebounceValue.ts @@ -17,8 +17,9 @@ * under the License. */ import { useState, useEffect } from 'react'; +import { FAST_DEBOUNCE } from 'src/constants'; -export function useDebounceValue(value: string, delay = 500) { +export function useDebounceValue(value: string, delay = FAST_DEBOUNCE) { const [debouncedValue, setDebouncedValue] = useState(value); useEffect(() => { From f905e200b6237c2c9bd3cca6bb32c8eb8407c701 Mon Sep 17 00:00:00 2001 From: justin-park Date: Fri, 5 May 2023 15:11:36 -0700 Subject: [PATCH 08/10] change describe wrapper to test --- .../src/hooks/useDebounceValue.test.ts | 94 +++++++++---------- 1 file changed, 45 insertions(+), 49 deletions(-) diff --git a/superset-frontend/src/hooks/useDebounceValue.test.ts b/superset-frontend/src/hooks/useDebounceValue.test.ts index ae0bdc2fcb316..b0bdff34ddc16 100644 --- a/superset-frontend/src/hooks/useDebounceValue.test.ts +++ b/superset-frontend/src/hooks/useDebounceValue.test.ts @@ -20,66 +20,62 @@ import { act, renderHook } from '@testing-library/react-hooks'; import { useDebounceValue } from './useDebounceValue'; -describe('useDebounceValue', () => { - afterEach(() => { - jest.clearAllTimers(); - jest.useRealTimers(); - }); - - it('should return the initial value', () => { - const { result } = renderHook(() => useDebounceValue('hello')); - expect(result.current).toBe('hello'); - }); +afterEach(() => { + jest.clearAllTimers(); + jest.useRealTimers(); +}); - it('should update debounced value after delay', async () => { - jest.useFakeTimers(); - const { result, rerender } = renderHook( - ({ value, delay }) => useDebounceValue(value, delay), - { initialProps: { value: 'hello', delay: 1000 } }, - ); +test('should return the initial value', () => { + const { result } = renderHook(() => useDebounceValue('hello')); + expect(result.current).toBe('hello'); +}); - expect(result.current).toBe('hello'); - act(() => { - rerender({ value: 'world', delay: 1000 }); - jest.advanceTimersByTime(500); - }); +test('should update debounced value after delay', async () => { + jest.useFakeTimers(); + const { result, rerender } = renderHook( + ({ value, delay }) => useDebounceValue(value, delay), + { initialProps: { value: 'hello', delay: 1000 } }, + ); - expect(result.current).toBe('hello'); + expect(result.current).toBe('hello'); + act(() => { + rerender({ value: 'world', delay: 1000 }); + jest.advanceTimersByTime(500); + }); - act(() => { - jest.advanceTimersByTime(1000); - }); + expect(result.current).toBe('hello'); - expect(result.current).toBe('world'); + act(() => { + jest.advanceTimersByTime(1000); }); - it('should cancel previous timeout when value changes', async () => { - jest.useFakeTimers(); - const { result, rerender } = renderHook( - ({ value, delay }) => useDebounceValue(value, delay), - { initialProps: { value: 'hello', delay: 1000 } }, - ); + expect(result.current).toBe('world'); +}); - expect(result.current).toBe('hello'); - rerender({ value: 'world', delay: 1000 }); +it('should cancel previous timeout when value changes', async () => { + jest.useFakeTimers(); + const { result, rerender } = renderHook( + ({ value, delay }) => useDebounceValue(value, delay), + { initialProps: { value: 'hello', delay: 1000 } }, + ); - jest.advanceTimersByTime(500); - rerender({ value: 'foo', delay: 1000 }); + expect(result.current).toBe('hello'); + rerender({ value: 'world', delay: 1000 }); - jest.advanceTimersByTime(500); - expect(result.current).toBe('hello'); - }); + jest.advanceTimersByTime(500); + rerender({ value: 'foo', delay: 1000 }); - it('should cancel the timeout when unmounting', async () => { - jest.useFakeTimers(); - const { result, unmount } = renderHook(() => - useDebounceValue('hello', 1000), - ); + jest.advanceTimersByTime(500); + expect(result.current).toBe('hello'); +}); - expect(result.current).toBe('hello'); - unmount(); +test('should cancel the timeout when unmounting', async () => { + jest.useFakeTimers(); + const { result, unmount } = renderHook(() => useDebounceValue('hello', 1000)); - jest.advanceTimersByTime(1000); - expect(clearTimeout).toHaveBeenCalled(); - }); + expect(result.current).toBe('hello'); + unmount(); + + jest.advanceTimersByTime(1000); + expect(clearTimeout).toHaveBeenCalled(); }); From 455a9e240809dab852879582463ab39533a3c59e Mon Sep 17 00:00:00 2001 From: justin-park Date: Fri, 5 May 2023 15:38:52 -0700 Subject: [PATCH 09/10] remove unnessassary styles --- .../src/components/FilterableTable/index.tsx | 57 ------------------- 1 file changed, 57 deletions(-) diff --git a/superset-frontend/src/components/FilterableTable/index.tsx b/superset-frontend/src/components/FilterableTable/index.tsx index fad52a66bb8b4..1cf880419db25 100644 --- a/superset-frontend/src/components/FilterableTable/index.tsx +++ b/superset-frontend/src/components/FilterableTable/index.tsx @@ -83,14 +83,6 @@ const StyledFilterableTable = styled.div` background-color: ${theme.colors.grayscale.light5}; } - .grid-header-cell { - display: inline-block; - max-width: 100%; - white-space: nowrap; - text-overflow: ellipsis; - overflow: hidden; - } - .ant-table-cell, .virtual-table-cell { min-width: 0px; @@ -98,42 +90,6 @@ const StyledFilterableTable = styled.div` font-size: ${theme.typography.sizes.s}px; } - .grid-header-cell { - font-weight: ${theme.typography.weights.bold}; - cursor: pointer; - } - - .ReactVirtualized__Table__headerColumn:last-of-type, - .ReactVirtualized__Table__rowColumn:last-of-type { - border-right: 0px; - } - - .ReactVirtualized__Table__headerColumn:focus, - .ReactVirtualized__Table__Grid:focus { - outline: none; - } - - .ReactVirtualized__Table__rowColumn { - text-overflow: ellipsis; - white-space: nowrap; - } - - .ReactVirtualized__Table__sortableHeaderColumn { - cursor: pointer; - } - - .ReactVirtualized__Table__sortableHeaderIconContainer { - display: flex; - align-items: center; - } - - .ReactVirtualized__Table__sortableHeaderIcon { - flex: 0 0 ${theme.gridUnit * 6}px; - height: 1em; - width: 1em; - fill: currentColor; - } - .even-row { background: ${theme.colors.grayscale.light4}; } @@ -142,19 +98,6 @@ const StyledFilterableTable = styled.div` background: ${theme.colors.grayscale.light5}; } - .header-style { - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - } - - .header-style-disabled { - overflow: hidden; - text-overflow: ellipsis; - white-space: nowrap; - color: ${theme.colors.grayscale.light1}; - } - .cell-text-for-measuring { font-family: ${theme.typography.families.sansSerif}; font-size: ${theme.typography.sizes.s}px; From daa6ee5a188965aa6bc28090fe789f882a6a52e1 Mon Sep 17 00:00:00 2001 From: justin-park Date: Thu, 11 May 2023 15:58:44 -0700 Subject: [PATCH 10/10] Fix Table spec related to AntD Table --- .../FilterableTable/FilterableTable.test.tsx | 427 ++++++++++-------- .../src/components/Table/index.tsx | 9 +- 2 files changed, 258 insertions(+), 178 deletions(-) diff --git a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx index 685019b294ed2..17e9cad2faa98 100644 --- a/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx +++ b/superset-frontend/src/components/FilterableTable/FilterableTable.test.tsx @@ -17,12 +17,10 @@ * under the License. */ import React from 'react'; -import { ReactWrapper } from 'enzyme'; -import { styledMount as mount } from 'spec/helpers/theming'; import FilterableTable, { convertBigIntStrToNumber, } from 'src/components/FilterableTable'; -import { render, screen } from 'spec/helpers/testing-library'; +import { render, screen, within } from 'spec/helpers/testing-library'; import userEvent from '@testing-library/user-event'; describe('FilterableTable', () => { @@ -35,279 +33,354 @@ describe('FilterableTable', () => { ], height: 500, }; - let wrapper: ReactWrapper; - beforeEach(() => { - wrapper = mount(); - }); it('is valid element', () => { expect(React.isValidElement()).toBe( true, ); }); - it('renders a grid with 2 Table rows', () => { - expect(wrapper.find('.ReactVirtualized__Grid')).toExist(); - expect(wrapper.find('.ReactVirtualized__Table__row')).toHaveLength(3); + it('renders a grid with 3 Table rows', () => { + const { getByRole, getByText } = render( + , + ); + expect(getByRole('table')).toBeInTheDocument(); + mockedProps.data.forEach(({ b: columnBContent }) => { + expect(getByText(columnBContent)).toBeInTheDocument(); + }); }); it('filters on a string', () => { const props = { ...mockedProps, filterText: 'b1', }; - wrapper = mount(); - expect(wrapper.find('.ReactVirtualized__Table__row')).toExist(); + const { getByText, queryByText } = render(); + expect(getByText(props.filterText)).toBeInTheDocument(); + expect(queryByText('b2')).toBeFalsy(); + expect(queryByText('b3')).toBeFalsy(); }); it('filters on a number', () => { const props = { ...mockedProps, filterText: '100', }; - wrapper = mount(); - expect(wrapper.find('.ReactVirtualized__Table__row')).toExist(); + const { getByText, queryByText } = render(); + expect(getByText('b2')).toBeInTheDocument(); + expect(queryByText('b1')).toBeFalsy(); + expect(queryByText('b3')).toBeFalsy(); }); }); describe('FilterableTable sorting - RTL', () => { it('sorts strings correctly', () => { const stringProps = { - orderedColumnKeys: ['a'], - data: [{ a: 'Bravo' }, { a: 'Alpha' }, { a: 'Charlie' }], + orderedColumnKeys: ['columnA'], + data: [ + { columnA: 'Bravo' }, + { columnA: 'Alpha' }, + { columnA: 'Charlie' }, + ], height: 500, }; render(); - const stringColumn = screen.getByRole('columnheader', { name: 'a' }); - const gridCells = screen.getAllByRole('gridcell'); + const stringColumn = within(screen.getByRole('table')) + .getByText('columnA') + .closest('th'); + // Antd 4.x Table does not follow the table role structure. Need a hacky selector to point the cell item + const gridCells = screen.getByTitle('Bravo').closest('.virtual-grid'); // Original order - expect(gridCells[0]).toHaveTextContent('Bravo'); - expect(gridCells[1]).toHaveTextContent('Alpha'); - expect(gridCells[2]).toHaveTextContent('Charlie'); + expect(gridCells?.textContent).toEqual( + ['Bravo', 'Alpha', 'Charlie'].join(''), + ); - // First click to sort ascending - userEvent.click(stringColumn); - expect(gridCells[0]).toHaveTextContent('Alpha'); - expect(gridCells[1]).toHaveTextContent('Bravo'); - expect(gridCells[2]).toHaveTextContent('Charlie'); + if (stringColumn) { + // First click to sort ascending + userEvent.click(stringColumn); + } - // Second click to sort descending - userEvent.click(stringColumn); - expect(gridCells[0]).toHaveTextContent('Charlie'); - expect(gridCells[1]).toHaveTextContent('Bravo'); - expect(gridCells[2]).toHaveTextContent('Alpha'); + expect(gridCells?.textContent).toEqual( + ['Alpha', 'Bravo', 'Charlie'].join(''), + ); - // Third click to clear sorting - userEvent.click(stringColumn); - expect(gridCells[0]).toHaveTextContent('Bravo'); - expect(gridCells[1]).toHaveTextContent('Alpha'); - expect(gridCells[2]).toHaveTextContent('Charlie'); + if (stringColumn) { + // Second click to sort descending + userEvent.click(stringColumn); + } + + expect(gridCells?.textContent).toEqual( + ['Charlie', 'Bravo', 'Alpha'].join(''), + ); + + if (stringColumn) { + // Third click to clear sorting + userEvent.click(stringColumn); + } + expect(gridCells?.textContent).toEqual( + ['Bravo', 'Alpha', 'Charlie'].join(''), + ); }); it('sorts integers correctly', () => { const integerProps = { - orderedColumnKeys: ['b'], - data: [{ b: 10 }, { b: 0 }, { b: 100 }], + orderedColumnKeys: ['columnB'], + data: [{ columnB: 21 }, { columnB: 0 }, { columnB: 623 }], height: 500, }; render(); - const integerColumn = screen.getByRole('columnheader', { name: 'b' }); - const gridCells = screen.getAllByRole('gridcell'); + const integerColumn = within(screen.getByRole('table')) + .getByText('columnB') + .closest('th'); + const gridCells = screen.getByTitle('21').closest('.virtual-grid'); // Original order - expect(gridCells[0]).toHaveTextContent('10'); - expect(gridCells[1]).toHaveTextContent('0'); - expect(gridCells[2]).toHaveTextContent('100'); + expect(gridCells?.textContent).toEqual(['21', '0', '623'].join('')); // First click to sort ascending - userEvent.click(integerColumn); - expect(gridCells[0]).toHaveTextContent('0'); - expect(gridCells[1]).toHaveTextContent('10'); - expect(gridCells[2]).toHaveTextContent('100'); + if (integerColumn) { + userEvent.click(integerColumn); + } + expect(gridCells?.textContent).toEqual(['0', '21', '623'].join('')); // Second click to sort descending - userEvent.click(integerColumn); - expect(gridCells[0]).toHaveTextContent('100'); - expect(gridCells[1]).toHaveTextContent('10'); - expect(gridCells[2]).toHaveTextContent('0'); + if (integerColumn) { + userEvent.click(integerColumn); + } + expect(gridCells?.textContent).toEqual(['623', '21', '0'].join('')); // Third click to clear sorting - userEvent.click(integerColumn); - expect(gridCells[0]).toHaveTextContent('10'); - expect(gridCells[1]).toHaveTextContent('0'); - expect(gridCells[2]).toHaveTextContent('100'); + if (integerColumn) { + userEvent.click(integerColumn); + } + expect(gridCells?.textContent).toEqual(['21', '0', '623'].join('')); }); it('sorts floating numbers correctly', () => { const floatProps = { - orderedColumnKeys: ['c'], - data: [{ c: 45.67 }, { c: 1.23 }, { c: 89.0000001 }], + orderedColumnKeys: ['columnC'], + data: [{ columnC: 45.67 }, { columnC: 1.23 }, { columnC: 89.0000001 }], height: 500, }; render(); - const floatColumn = screen.getByRole('columnheader', { name: 'c' }); - const gridCells = screen.getAllByRole('gridcell'); + const floatColumn = within(screen.getByRole('table')) + .getByText('columnC') + .closest('th'); + const gridCells = screen.getByTitle('45.67').closest('.virtual-grid'); // Original order - expect(gridCells[0]).toHaveTextContent('45.67'); - expect(gridCells[1]).toHaveTextContent('1.23'); - expect(gridCells[2]).toHaveTextContent('89.0000001'); + expect(gridCells?.textContent).toEqual( + ['45.67', '1.23', '89.0000001'].join(''), + ); // First click to sort ascending - userEvent.click(floatColumn); - expect(gridCells[0]).toHaveTextContent('1.23'); - expect(gridCells[1]).toHaveTextContent('45.67'); - expect(gridCells[2]).toHaveTextContent('89.0000001'); + if (floatColumn) { + userEvent.click(floatColumn); + } + expect(gridCells?.textContent).toEqual( + ['1.23', '45.67', '89.0000001'].join(''), + ); // Second click to sort descending - userEvent.click(floatColumn); - expect(gridCells[0]).toHaveTextContent('89.0000001'); - expect(gridCells[1]).toHaveTextContent('45.67'); - expect(gridCells[2]).toHaveTextContent('1.23'); + if (floatColumn) { + userEvent.click(floatColumn); + } + expect(gridCells?.textContent).toEqual( + ['89.0000001', '45.67', '1.23'].join(''), + ); // Third click to clear sorting - userEvent.click(floatColumn); - expect(gridCells[0]).toHaveTextContent('45.67'); - expect(gridCells[1]).toHaveTextContent('1.23'); - expect(gridCells[2]).toHaveTextContent('89.0000001'); + if (floatColumn) { + userEvent.click(floatColumn); + } + expect(gridCells?.textContent).toEqual( + ['45.67', '1.23', '89.0000001'].join(''), + ); }); it('sorts rows properly when floating numbers have mixed types', () => { const mixedFloatProps = { - orderedColumnKeys: ['d'], + orderedColumnKeys: ['columnD'], data: [ - { d: 48710.92 }, - { d: 145776.56 }, - { d: 72212.86 }, - { d: '144729.96000000002' }, - { d: '26260.210000000003' }, - { d: '152718.97999999998' }, - { d: 28550.59 }, - { d: '24078.610000000004' }, - { d: '98089.08000000002' }, - { d: '3439718.0300000007' }, - { d: '4528047.219999993' }, + { columnD: 48710.92 }, + { columnD: 145776.56 }, + { columnD: 72212.86 }, + { columnD: '144729.96000000002' }, + { columnD: '26260.210000000003' }, + { columnD: '152718.97999999998' }, + { columnD: 28550.59 }, + { columnD: '24078.610000000004' }, + { columnD: '98089.08000000002' }, + { columnD: '3439718.0300000007' }, + { columnD: '4528047.219999993' }, ], height: 500, }; render(); - const mixedFloatColumn = screen.getByRole('columnheader', { name: 'd' }); - const gridCells = screen.getAllByRole('gridcell'); + const mixedFloatColumn = within(screen.getByRole('table')) + .getByText('columnD') + .closest('th'); + const gridCells = screen.getByTitle('48710.92').closest('.virtual-grid'); // Original order - expect(gridCells[0]).toHaveTextContent('48710.92'); - expect(gridCells[1]).toHaveTextContent('145776.56'); - expect(gridCells[2]).toHaveTextContent('72212.86'); - expect(gridCells[3]).toHaveTextContent('144729.96000000002'); - expect(gridCells[4]).toHaveTextContent('26260.210000000003'); - expect(gridCells[5]).toHaveTextContent('152718.97999999998'); - expect(gridCells[6]).toHaveTextContent('28550.59'); - expect(gridCells[7]).toHaveTextContent('24078.610000000004'); - expect(gridCells[8]).toHaveTextContent('98089.08000000002'); - expect(gridCells[9]).toHaveTextContent('3439718.0300000007'); - expect(gridCells[10]).toHaveTextContent('4528047.219999993'); - + expect(gridCells?.textContent).toEqual( + [ + '48710.92', + '145776.56', + '72212.86', + '144729.96000000002', + '26260.210000000003', + '152718.97999999998', + '28550.59', + '24078.610000000004', + '98089.08000000002', + '3439718.0300000007', + '4528047.219999993', + ].join(''), + ); // First click to sort ascending - userEvent.click(mixedFloatColumn); - expect(gridCells[0]).toHaveTextContent('24078.610000000004'); - expect(gridCells[1]).toHaveTextContent('26260.210000000003'); - expect(gridCells[2]).toHaveTextContent('28550.59'); - expect(gridCells[3]).toHaveTextContent('48710.92'); - expect(gridCells[4]).toHaveTextContent('72212.86'); - expect(gridCells[5]).toHaveTextContent('98089.08000000002'); - expect(gridCells[6]).toHaveTextContent('144729.96000000002'); - expect(gridCells[7]).toHaveTextContent('145776.56'); - expect(gridCells[8]).toHaveTextContent('152718.97999999998'); - expect(gridCells[9]).toHaveTextContent('3439718.0300000007'); - expect(gridCells[10]).toHaveTextContent('4528047.219999993'); + if (mixedFloatColumn) { + userEvent.click(mixedFloatColumn); + } + expect(gridCells?.textContent).toEqual( + [ + '24078.610000000004', + '26260.210000000003', + '28550.59', + '48710.92', + '72212.86', + '98089.08000000002', + '144729.96000000002', + '145776.56', + '152718.97999999998', + '3439718.0300000007', + '4528047.219999993', + ].join(''), + ); // Second click to sort descending - userEvent.click(mixedFloatColumn); - expect(gridCells[0]).toHaveTextContent('4528047.219999993'); - expect(gridCells[1]).toHaveTextContent('3439718.0300000007'); - expect(gridCells[2]).toHaveTextContent('152718.97999999998'); - expect(gridCells[3]).toHaveTextContent('145776.56'); - expect(gridCells[4]).toHaveTextContent('144729.96000000002'); - expect(gridCells[5]).toHaveTextContent('98089.08000000002'); - expect(gridCells[6]).toHaveTextContent('72212.86'); - expect(gridCells[7]).toHaveTextContent('48710.92'); - expect(gridCells[8]).toHaveTextContent('28550.59'); - expect(gridCells[9]).toHaveTextContent('26260.210000000003'); - expect(gridCells[10]).toHaveTextContent('24078.610000000004'); + if (mixedFloatColumn) { + userEvent.click(mixedFloatColumn); + } + expect(gridCells?.textContent).toEqual( + [ + '4528047.219999993', + '3439718.0300000007', + '152718.97999999998', + '145776.56', + '144729.96000000002', + '98089.08000000002', + '72212.86', + '48710.92', + '28550.59', + '26260.210000000003', + '24078.610000000004', + ].join(''), + ); // Third click to clear sorting - userEvent.click(mixedFloatColumn); - expect(gridCells[0]).toHaveTextContent('48710.92'); - expect(gridCells[1]).toHaveTextContent('145776.56'); - expect(gridCells[2]).toHaveTextContent('72212.86'); - expect(gridCells[3]).toHaveTextContent('144729.96000000002'); - expect(gridCells[4]).toHaveTextContent('26260.210000000003'); - expect(gridCells[5]).toHaveTextContent('152718.97999999998'); - expect(gridCells[6]).toHaveTextContent('28550.59'); - expect(gridCells[7]).toHaveTextContent('24078.610000000004'); - expect(gridCells[8]).toHaveTextContent('98089.08000000002'); - expect(gridCells[9]).toHaveTextContent('3439718.0300000007'); - expect(gridCells[10]).toHaveTextContent('4528047.219999993'); + if (mixedFloatColumn) { + userEvent.click(mixedFloatColumn); + } + expect(gridCells?.textContent).toEqual( + [ + '48710.92', + '145776.56', + '72212.86', + '144729.96000000002', + '26260.210000000003', + '152718.97999999998', + '28550.59', + '24078.610000000004', + '98089.08000000002', + '3439718.0300000007', + '4528047.219999993', + ].join(''), + ); }); it('sorts YYYY-MM-DD properly', () => { const dsProps = { - orderedColumnKeys: ['ds'], + orderedColumnKeys: ['columnDS'], data: [ - { ds: '2021-01-01' }, - { ds: '2022-01-01' }, - { ds: '2021-01-02' }, - { ds: '2021-01-03' }, - { ds: '2021-12-01' }, - { ds: '2021-10-01' }, - { ds: '2022-01-02' }, + { columnDS: '2021-01-01' }, + { columnDS: '2022-01-01' }, + { columnDS: '2021-01-02' }, + { columnDS: '2021-01-03' }, + { columnDS: '2021-12-01' }, + { columnDS: '2021-10-01' }, + { columnDS: '2022-01-02' }, ], height: 500, }; render(); - const dsColumn = screen.getByRole('columnheader', { name: 'ds' }); - const gridCells = screen.getAllByRole('gridcell'); + const dsColumn = within(screen.getByRole('table')) + .getByText('columnDS') + .closest('th'); + const gridCells = screen.getByTitle('2021-01-01').closest('.virtual-grid'); // Original order - expect(gridCells[0]).toHaveTextContent('2021-01-01'); - expect(gridCells[1]).toHaveTextContent('2022-01-01'); - expect(gridCells[2]).toHaveTextContent('2021-01-02'); - expect(gridCells[3]).toHaveTextContent('2021-01-03'); - expect(gridCells[4]).toHaveTextContent('2021-12-01'); - expect(gridCells[5]).toHaveTextContent('2021-10-01'); - expect(gridCells[6]).toHaveTextContent('2022-01-02'); + expect(gridCells?.textContent).toEqual( + [ + '2021-01-01', + '2022-01-01', + '2021-01-02', + '2021-01-03', + '2021-12-01', + '2021-10-01', + '2022-01-02', + ].join(''), + ); // First click to sort ascending - userEvent.click(dsColumn); - expect(gridCells[0]).toHaveTextContent('2021-01-01'); - expect(gridCells[1]).toHaveTextContent('2021-01-02'); - expect(gridCells[2]).toHaveTextContent('2021-01-03'); - expect(gridCells[3]).toHaveTextContent('2021-10-01'); - expect(gridCells[4]).toHaveTextContent('2021-12-01'); - expect(gridCells[5]).toHaveTextContent('2022-01-01'); - expect(gridCells[6]).toHaveTextContent('2022-01-02'); + if (dsColumn) { + userEvent.click(dsColumn); + } + expect(gridCells?.textContent).toEqual( + [ + '2021-01-01', + '2021-01-02', + '2021-01-03', + '2021-10-01', + '2021-12-01', + '2022-01-01', + '2022-01-02', + ].join(''), + ); // Second click to sort descending - userEvent.click(dsColumn); - expect(gridCells[0]).toHaveTextContent('2022-01-02'); - expect(gridCells[1]).toHaveTextContent('2022-01-01'); - expect(gridCells[2]).toHaveTextContent('2021-12-01'); - expect(gridCells[3]).toHaveTextContent('2021-10-01'); - expect(gridCells[4]).toHaveTextContent('2021-01-03'); - expect(gridCells[5]).toHaveTextContent('2021-01-02'); - expect(gridCells[6]).toHaveTextContent('2021-01-01'); + if (dsColumn) { + userEvent.click(dsColumn); + } + expect(gridCells?.textContent).toEqual( + [ + '2022-01-02', + '2022-01-01', + '2021-12-01', + '2021-10-01', + '2021-01-03', + '2021-01-02', + '2021-01-01', + ].join(''), + ); // Third click to clear sorting - userEvent.click(dsColumn); - expect(gridCells[0]).toHaveTextContent('2021-01-01'); - expect(gridCells[1]).toHaveTextContent('2022-01-01'); - expect(gridCells[2]).toHaveTextContent('2021-01-02'); - expect(gridCells[3]).toHaveTextContent('2021-01-03'); - expect(gridCells[4]).toHaveTextContent('2021-12-01'); - expect(gridCells[5]).toHaveTextContent('2021-10-01'); - expect(gridCells[6]).toHaveTextContent('2022-01-02'); + if (dsColumn) { + userEvent.click(dsColumn); + } + expect(gridCells?.textContent).toEqual( + [ + '2021-01-01', + '2022-01-01', + '2021-01-02', + '2021-01-03', + '2021-12-01', + '2021-10-01', + '2022-01-02', + ].join(''), + ); }); }); diff --git a/superset-frontend/src/components/Table/index.tsx b/superset-frontend/src/components/Table/index.tsx index 29a512093302c..e5ce24c64eb20 100644 --- a/superset-frontend/src/components/Table/index.tsx +++ b/superset-frontend/src/components/Table/index.tsx @@ -397,7 +397,14 @@ export function Table( {virtualize && ( )}