[GitHub] [superset] michael-s-molina commented on a diff in pull request #23035: feat: Use AntD table in FilterableTable

["michael-s-molina (via GitHub)" <gi...@apache.org>]

michael-s-molina commented on code in PR #23035:
URL: https://github.com/apache/superset/pull/23035#discussion_r1174586978


##########
superset-frontend/src/components/FilterableTable/index.tsx:
##########
@@ -377,6 +357,7 @@ const FilterableTable = ({
     return values.some(v => v.includes(lowerCaseText));
   };
 
+  // @ts-ignore

Review Comment:
   Why are you ignoring this variable instead of removing it?



##########
superset-frontend/src/components/FilterableTable/index.tsx:
##########
@@ -188,9 +178,6 @@ const StyledFilterableTable = styled.div`
   `}
 `;
 
-// when more than MAX_COLUMNS_FOR_TABLE are returned, switch from table to grid view

Review Comment:
   Can you remove unused properties?
   - `headerHeight`
   - `overscanColumnCount`
   - `overscanRowCount`
   - `rowHeight`
   
   and fix the `'getCellContent' was used before it was defined` lint error?
   



##########
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();

Review Comment:
   ```suggestion
       const { getByRole } = setup(props, mockStore(initialState));
       expect(getByRole('table')).toBeInTheDocument();
   ```



##########
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', () => {

Review Comment:
   You should also replace other occurrences of `grid` by `table`.



##########
superset-frontend/src/SqlLab/components/ResultSet/index.tsx:
##########
@@ -483,7 +476,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

Review Comment:
   It's better to just reference the Table component and leave the `antd` part out because in the future we can change our table component to use another implementation and this comment will still be valid.
   
   ```suggestion
       // the Table component
   ```



##########
superset-frontend/src/components/FilterableTable/index.tsx:
##########
@@ -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,

Review Comment:
   Can we delete `StyledFilterableTable`?  I'm assuming that the default theme of our table component is sufficient. If not, can you adjust its class references? `.ReactVirtualized_` classes don't even exist in the new component.



##########
superset-frontend/src/SqlLab/components/ResultSet/ResultSet.test.tsx:
##########
@@ -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();

Review Comment:
   ```suggestion
       expect(screen.getByRole('table')).toBeVisible();
   ```



##########
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();

Review Comment:
   It's important to follow [RTL Guiding principles](https://testing-library.com/docs/queries/about#priority) and use `getByTestId` as the last resort.
   
   ```suggestion
       const { getByRole } = setup(mockedProps, mockStore(initialState));
       expect(getByRole('table')).toBeInTheDocument();
   ```



##########
superset-frontend/src/components/FilterableTable/index.tsx:
##########
@@ -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 = (

Review Comment:
   Can you rename `addJsonModal` to `jsonModal` or `newJsonModal`. This function is not adding anything, it's returning a modal instance.



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