refactor: decouple DataTableControl #20226

zhaoyongjie · 2022-05-31T09:11:19Z

SUMMARY

The DataTableControl is a component in DataTablePane, it has an important function is to switch the format of the time columns. Currently, this toggle was implemented by Redux, and saved state in the Explore's store.

We intend to move DataPane into Dashboard(PR), the Dashboard store and the Explore store are separated store, so I have to remove the time column state from the explore state.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

dataTableControl.mov

copy to clipboard

copy.to.clipboard.mov

TESTING INSTRUCTIONS

The DataTablePane work as before.
Copy to clipboard work as expected.

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

codecov · 2022-05-31T10:31:13Z

Codecov Report

Merging #20226 (ca73969) into master (3d5ae62) will increase coverage by 0.02%.
The diff coverage is 50.00%.

@@            Coverage Diff             @@
##           master   #20226      +/-   ##
==========================================
+ Coverage   66.48%   66.51%   +0.02%     
==========================================
  Files        1726     1726              
  Lines       64759    64763       +4     
  Branches     6828     6829       +1     
==========================================
+ Hits        43058    43074      +16     
+ Misses      19966    19958       -8     
+ Partials     1735     1731       -4

Flag	Coverage Δ
javascript	`51.36% <50.00%> (+0.04%)`	⬆️

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

Impacted Files	Coverage Δ
...set-frontend/src/explore/actions/exploreActions.ts	`50.00% <ø> (ø)`
...mponents/DataTablesPane/components/ResultsPane.tsx	`58.18% <ø> (-0.75%)`	⬇️
...mponents/DataTablesPane/components/SamplesPane.tsx	`69.76% <ø> (-0.69%)`	⬇️
...et-frontend/src/explore/reducers/exploreReducer.js	`35.93% <ø> (+7.18%)`	⬆️
...t-frontend/src/explore/reducers/getInitialState.ts	`0.00% <ø> (ø)`
.../src/explore/components/DataTableControl/index.tsx	`64.04% <32.00%> (-5.19%)`	⬇️
...d/src/explore/components/DataTableControl/utils.ts	`75.00% <75.00%> (ø)`
...ts/DataTablesPane/components/DataTableControls.tsx	`100.00% <100.00%> (ø)`
...rset-frontend/src/explore/exploreUtils/formData.ts	`85.71% <0.00%> (ø)`
.../src/dashboard/components/gridComponents/Chart.jsx	`60.60% <0.00%> (ø)`
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3d5ae62...ca73969. Read the comment docs.

zhaoyongjie · 2022-05-31T11:27:14Z

superset-frontend/src/explore/components/DataTableControl/index.tsx

@@ -130,8 +126,8 @@ export const RowCount = ({
 }) => <RowCountLabel rowcount={data?.length ?? 0} loading={loading} />;

 enum FormatPickerValue {
-  Formatted,
-  Original,
+  Formatted = 'formatted',


The string enum type is more readable in the debugger.

zhaoyongjie · 2022-05-31T11:28:46Z

superset-frontend/src/explore/components/DataTableControl/index.tsx

-  const dispatch = useDispatch();
-  const isTimeColumnOriginalFormatted = originalFormattedTimeColumnIndex > -1;
-
-  const onChange = useCallback(


the same logic moves to parent's component.

fix lint typing

zhaoyongjie · 2022-06-02T02:22:56Z

/testenv up

github-actions · 2022-06-02T02:24:59Z

@zhaoyongjie Ephemeral environment spinning up at http://34.222.134.57:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

superset-frontend/src/explore/components/DataTablesPane/components/DataTableControls.tsx

kgabryje · 2022-06-02T18:06:08Z

superset-frontend/src/explore/components/DataTableControl/utils.ts

-    datasourceId
-      ? state?.explore?.originalFormattedTimeColumns?.[datasourceId] ?? []
-      : [],
+export const getTimeColumns = (datasourceId?: string): string[] => {


I think the name suggests that the function gets all time columns, not only the time columns with epoch formatting in data table

The time columns means that the column type is GenericDataType.Temporal after Superset introduced generic data type.

kgabryje · 2022-06-02T18:06:17Z

superset-frontend/src/explore/components/DataTableControl/utils.ts

+  return ensureIsArray(colsMap[datasourceId]);
+};
+
+export const setTimeColumns = (datasourceId: string, columns: string[]) => {


Same as above

kgabryje

Looks great! I commented on some minor nitpicks

github-actions · 2022-06-03T01:53:20Z

Ephemeral environment shutdown and build artifacts deleted.

pull-request-size bot added the size/L label May 31, 2022

zhaoyongjie marked this pull request as ready for review May 31, 2022 09:11

zhaoyongjie requested review from kgabryje and villebro and removed request for kgabryje May 31, 2022 10:09

zhaoyongjie commented May 31, 2022

View reviewed changes

refactor: decouple datatable controls

4e7fc70

fix lint typing

zhaoyongjie force-pushed the refactor/datatable_control branch from 506a48f to 4e7fc70 Compare June 1, 2022 12:42

zhaoyongjie requested a review from stephenLYZ June 1, 2022 12:44

zhaoyongjie mentioned this pull request Jun 1, 2022

feat: showing results pane in dashboard #20144

Merged

9 tasks

kgabryje reviewed Jun 2, 2022

View reviewed changes

superset-frontend/src/explore/components/DataTablesPane/components/DataTableControls.tsx Outdated Show resolved Hide resolved

kgabryje reviewed Jun 2, 2022

View reviewed changes

kgabryje approved these changes Jun 2, 2022

View reviewed changes

fix typos

ca73969

zhaoyongjie merged commit a020f75 into apache:master Jun 3, 2022

philipher29 pushed a commit to ValtechMobility/superset that referenced this pull request Jun 9, 2022

refactor: decouple DataTableControl (apache#20226)

45d46aa

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: decouple DataTableControl #20226

refactor: decouple DataTableControl #20226

zhaoyongjie commented May 31, 2022 •

edited

Loading

codecov bot commented May 31, 2022 •

edited

Loading

zhaoyongjie May 31, 2022 •

edited

Loading

zhaoyongjie May 31, 2022

zhaoyongjie commented Jun 2, 2022

github-actions bot commented Jun 2, 2022

kgabryje Jun 2, 2022

zhaoyongjie Jun 3, 2022

kgabryje Jun 2, 2022

kgabryje left a comment

github-actions bot commented Jun 3, 2022

refactor: decouple DataTableControl #20226

refactor: decouple DataTableControl #20226

Conversation

zhaoyongjie commented May 31, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After

copy to clipboard

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented May 31, 2022 • edited Loading

Codecov Report

zhaoyongjie May 31, 2022 • edited Loading

Choose a reason for hiding this comment

zhaoyongjie May 31, 2022

Choose a reason for hiding this comment

zhaoyongjie commented Jun 2, 2022

github-actions bot commented Jun 2, 2022

kgabryje Jun 2, 2022

Choose a reason for hiding this comment

zhaoyongjie Jun 3, 2022

Choose a reason for hiding this comment

kgabryje Jun 2, 2022

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 3, 2022

zhaoyongjie commented May 31, 2022 •

edited

Loading

codecov bot commented May 31, 2022 •

edited

Loading

zhaoyongjie May 31, 2022 •

edited

Loading