feat: Use AntD table in FilterableTable by EugeneTorap · Pull Request #23035 · apache/superset

EugeneTorap · 2023-02-08T18:23:31Z

SUMMARY

react-virtualized does not support for react 17 and react 18.
This PR is:

Removing react-virtualized lib.
Refactoring FilterableTable to use our AntD table.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

lilykuang · 2023-02-21T22:13:27Z

LGTM. I'd like to get more eyes on this PR. requested more reviewers

Antonio-RiveroMartnez · 2023-04-21T16:22:51Z

Wondering if we need this PR or just bump to latest version that includes react 17 and 18? https://github.com/bvaughn/react-virtualized/releases/tag/v9.22.4 @EugeneTorap @lilykuang

EugeneTorap · 2023-04-21T17:02:56Z

@Antonio-RiveroMartnez I think this lib is useless for us we should drop it and use only antd table here

michael-s-molina

Thank you for the PR @EugeneTorap. I left-some first-pass comments.

michael-s-molina · 2023-04-23T14:46:26Z

Can you also debounce the search in SQL Lab? It's re-rendering the table for each typed character.

Screen.Recording.2023-04-23.at.11.44.38.mov

michael-s-molina · 2023-04-24T00:59:14Z

I fixed the Frontend CI error here

justinpark · 2023-05-04T21:07:34Z

@michael-s-molina I cleaned up all addressed issues.

I also added the debounced search including loading indicator for better UX

debounced_search.mov

cc: @EugeneTorap

michael-s-molina · 2023-05-05T12:51:18Z

-    .ReactVirtualized__Table__rowColumn,
-    .grid-cell {
+    .ant-table-cell,
+    .virtual-table-cell {


@justinpark @EugeneTorap Can you provide some context why the default Table theming is not sufficient? The reason I'm asking is because one of the main reason we have a table component is to make sure all tables look and behave the same throughout the application. What are the behaviors that the default Table is missing? Can we implement them on the default Table?

Good point. There's a bunch of differences between this update and existing one.

screenshot

Existing one

Table component (no stripes, bigger font size, bigger padding)

Table with custom style (no stripes, bigger padding but same font size)

The major style update is font-size since it impacts the number of visible rows due to increased row height.
I can add this custom style on default Table if applicable for other cases.

I like how Table with custom style looks!

For me, the important part is that tables look consistent. So I'm ok with either option as long as it's applied globally to the table component and not only to SQL Lab. To make this type of decision, we need to consider the table use cases globally. I think @kasiazjc should be the one to decide which style is more appropriate.

@kasiazjc could you give us an input for design side?

michael-s-molina · 2023-05-05T12:57:49Z

@michael-s-molina I cleaned up all addressed issues.

I also added the debounced search including loading indicator for better UX

Awesome! Thank you for the changes. I left second review comments.

codecov · 2023-05-12T00:07:19Z

Codecov Report

Merging #23035 (b1dcddb) into master (fd3030f) will decrease coverage by 1.57%.
The diff coverage is 85.29%.

❗ Current head b1dcddb differs from pull request most recent head daa6ee5. Consider uploading reports for the commit daa6ee5 to get more accurate results

@@            Coverage Diff             @@
##           master   #23035      +/-   ##
==========================================
- Coverage   68.11%   66.55%   -1.57%     
==========================================
  Files        1938     1942       +4     
  Lines       74973    75224     +251     
  Branches     8141     8145       +4     
==========================================
- Hits        51068    50063    -1005     
- Misses      21826    23076    +1250     
- Partials     2079     2085       +6

Flag	Coverage Δ
javascript	`54.52% <63.76%> (+0.04%)`	⬆️
presto	`?`
unit	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...chart-controls/src/shared-controls/dndControls.tsx	`58.33% <ø> (ø)`
...ackages/superset-ui-core/src/utils/featureFlags.ts	`100.00% <ø> (ø)`
...s/legacy-plugin-chart-calendar/src/controlPanel.ts	`50.00% <ø> (ø)`
...rts/src/BigNumber/BigNumberTotal/transformProps.ts	`0.00% <0.00%> (ø)`
...lugin-chart-echarts/src/BigNumber/BigNumberViz.tsx	`0.00% <0.00%> (ø)`
...chart-echarts/src/MixedTimeseries/controlPanel.tsx	`56.52% <ø> (ø)`
...hart-echarts/src/MixedTimeseries/transformProps.ts	`0.00% <0.00%> (ø)`
superset-frontend/src/SqlLab/App.jsx	`0.00% <0.00%> (ø)`
superset-frontend/src/SqlLab/actions/sqlLab.js	`66.41% <ø> (+0.08%)`	⬆️
...frontend/src/SqlLab/components/ResultSet/index.tsx	`63.05% <ø> (+0.39%)`	⬆️
... and 114 more

... and 88 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

EugeneTorap · 2023-05-12T18:15:39Z

Hi @michael-s-molina! Can we merge this PR in order to unblock the PR of ReactJS 17 support.
We will improve styles of the table in SQL Lab in next PR.

michael-s-molina

LGTM with the condition of revisiting the Table style in a follow-up. @kasiazjc

Co-authored-by: justin-park <justin.park@airbnb.com>

pull-request-size Bot added the size/L label Feb 8, 2023

EugeneTorap force-pushed the feat/use-antd-table-in-filterable-table branch from e60d7f3 to 5fdff26 Compare February 18, 2023 18:00

EugeneTorap marked this pull request as ready for review February 18, 2023 19:26

EugeneTorap force-pushed the feat/use-antd-table-in-filterable-table branch from ea1cfef to 97f9e5f Compare February 19, 2023 08:28

EugeneTorap mentioned this pull request Feb 20, 2023

fix: Pagination of VirtualTable #23130

Merged

9 tasks

lilykuang requested review from etr2460, geido, kgabryje and villebro February 21, 2023 22:12

eschutho requested a review from lilykuang March 27, 2023 16:56

Antonio-RiveroMartnez approved these changes Apr 21, 2023

View reviewed changes

rusackas requested a review from michael-s-molina April 23, 2023 02:25

michael-s-molina requested changes Apr 23, 2023

View reviewed changes

Use antd table in FilterableTable

f915aac

EugeneTorap force-pushed the feat/use-antd-table-in-filterable-table branch from 97f9e5f to f915aac Compare May 1, 2023 11:05

justinpark added 4 commits May 4, 2023 10:18

update package-lock

c972f1c

replace to getByRole

70139fb

update comments

91c67bb

clean up addressed issues including debounced search

51c2302

pull-request-size Bot added size/XL and removed size/L labels May 4, 2023

remove unused

8cf23a4

michael-s-molina reviewed May 5, 2023

View reviewed changes

Comment thread superset-frontend/src/hooks/useDebounceValue.test.ts Outdated

Comment thread superset-frontend/src/hooks/useDebounceValue.ts Outdated

justinpark added 4 commits May 5, 2023 15:10

update FAST_DEBOUNCE reference

7e5aa70

change describe wrapper to test

f905e20

remove unnessassary styles

455a9e2

Fix Table spec related to AntD Table

daa6ee5

pull-request-size Bot added size/XXL and removed size/XL labels May 11, 2023

michael-s-molina approved these changes May 12, 2023

View reviewed changes

michael-s-molina merged commit 1670275 into apache:master May 12, 2023

EugeneTorap deleted the feat/use-antd-table-in-filterable-table branch May 12, 2023 20:54

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 First shipped in 3.0.0 labels Mar 13, 2024

qfcwell pushed a commit to qfcwell/superset that referenced this pull request May 12, 2026

feat: Use AntD table in FilterableTable (apache#23035)

d6a09fe

Co-authored-by: justin-park <justin.park@airbnb.com>

	screenshot
Existing one
Table component (no stripes, bigger font size, bigger padding)
Table with custom style (no stripes, bigger padding but same font size)

Conversation

EugeneTorap commented Feb 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Uh oh!

lilykuang commented Feb 21, 2023

Uh oh!

Antonio-RiveroMartnez commented Apr 21, 2023

Uh oh!

EugeneTorap commented Apr 21, 2023

Uh oh!

michael-s-molina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

michael-s-molina commented Apr 23, 2023

Uh oh!

michael-s-molina commented Apr 24, 2023

Uh oh!

justinpark commented May 4, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

michael-s-molina May 5, 2023

Choose a reason for hiding this comment

Uh oh!

justinpark May 5, 2023

Choose a reason for hiding this comment

Uh oh!

EugeneTorap May 6, 2023

Choose a reason for hiding this comment

Uh oh!

michael-s-molina May 8, 2023

Choose a reason for hiding this comment

Uh oh!

justinpark May 11, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

michael-s-molina commented May 5, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented May 12, 2023

Codecov Report

Uh oh!

EugeneTorap commented May 12, 2023

Uh oh!

michael-s-molina left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

EugeneTorap commented Feb 8, 2023 •

edited

Loading

justinpark commented May 4, 2023 •

edited

Loading

michael-s-molina commented May 5, 2023 •

edited

Loading

michael-s-molina left a comment •

edited

Loading