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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
