fix(datasets): Replace left panel layout by TableSelector #24599

justinpark · 2023-07-05T22:58:00Z

SUMMARY

Since new Dataset creation layout doesn't use virtualized list component for the table list, hard to operate the dataset creation with large table set (>10k). (mostly browser is frozen while scrolling down)

Since the left panel layout designed for database/schema/table selection, we can reuse the database/schema/table selector (which uses virtualized list) for the same functionality (except the warning icon. However the warning message will be shown as is once user selects a table. I think it won't be critical without the warning icon on the list)

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

After:

Before:

TESTING INSTRUCTIONS

Go to datasets and create new dataset

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

john-bodley · 2023-07-05T23:05:09Z

@justinpark regarding the lack of the warning icon, even if one perceives this as a regression, I sense that overall (performance aside) the replacement is actually net positive as it ensures a consistent UI/UX between the two workflows (and less code to maintain).

codecov · 2023-07-06T01:12:39Z

Codecov Report

Merging #24599 (1ec2789) into master (1a97245) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

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

@@            Coverage Diff             @@
##           master   #24599      +/-   ##
==========================================
- Coverage   68.89%   68.89%   -0.01%     
==========================================
  Files        1901     1901              
  Lines       73925    73863      -62     
  Branches     8183     8173      -10     
==========================================
- Hits        50931    50886      -45     
+ Misses      20873    20862      -11     
+ Partials     2121     2115       -6

Flag	Coverage Δ
javascript	`55.68% <100.00%> (-0.03%)`	⬇️

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

Impacted Files	Coverage Δ
superset-frontend/src/components/Tooltip/index.tsx	`100.00% <ø> (ø)`
...et-frontend/src/components/TableSelector/index.tsx	`86.66% <100.00%> (+5.58%)`	⬆️
...nd/src/components/WarningIconWithTooltip/index.tsx	`100.00% <100.00%> (ø)`
...c/features/datasets/AddDataset/LeftPanel/index.tsx	`84.21% <100.00%> (+5.63%)`	⬆️
...end/src/features/datasets/hooks/useDatasetLists.ts	`33.33% <100.00%> (+2.89%)`	⬆️

... and 2 files with indirect coverage changes

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

eschutho · 2023-07-06T23:41:13Z

/testenv up

github-actions · 2023-07-06T23:43:06Z

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

yousoph · 2023-07-07T00:53:58Z

I do think the icon is helpful to understand which tables are already backing datasets without having to click into each one. Is there some way to still display that info/indicator on the list?

kasiazjc · 2023-07-07T10:02:37Z

I do think the icon is helpful to understand which tables are already backing datasets without having to click into each one. Is there some way to still display that info/indicator on the list?

Agree with @yousoph. I think there should be some kind of indicator in the dropdown that the table is already used in a different dataset especially as the table can be only used for one dataset (if I understand correctly).

michael-s-molina · 2023-07-07T11:48:00Z

Is there some way to still display that info/indicator on the list?

@justinpark You can use customLabel to change how the Select options are displayed and include the icon.

justinpark · 2023-07-07T18:59:01Z

@yousoph @kasiazjc @michael-s-molina As discussed, I restored the warning icon. it's not identical but reuse existing warning indicator in TableOption component.

justinpark · 2023-07-07T19:32:14Z

Adjusted the right margin of the warning icon

michael-s-molina · 2023-07-07T19:57:27Z

superset-frontend/src/features/datasets/AddDataset/LeftPanel/index.tsx

-      <Loading position="inline" />
-      <p>{inline}</p>
-    </div>
+  const datasetNamesSet = datasetNames?.join(divider);


Can you use a Set here?

The purpose of using concatenated string is avoiding the re-rendering by the reference differences because it will shallowly comparing only values given the different reference of datasetNames passing from the property

Why not use Lodash's isEqual to compare the sets? split and includes for every table seems way more expensive.

I removed the join/split operation and fix the original hook for tuning

superset-frontend/src/components/WarningIconWithTooltip/index.tsx

yousoph · 2023-07-07T21:56:20Z

Thanks @justinpark ! One small item - in your screenshot, the tooltip looks like it has a bit too much space underneath the text

justinpark · 2023-07-11T16:43:09Z

@yousoph I just removed the minor bottom margin on the tooltip.

justinpark · 2023-07-14T16:21:31Z

@michael-s-molina I made updates on the addressed ones.
Could you help review?

michael-s-molina · 2023-07-19T14:13:19Z

superset-frontend/src/components/WarningIconWithTooltip/index.tsx

@@ -24,11 +24,13 @@ import { Tooltip } from 'src/components/Tooltip';
 export interface WarningIconWithTooltipProps {
  warningMarkdown: string;
  size?: IconType['iconSize'];
+  indentSize?: number;


Can you pass marginRight as the property instead of indentSize and use theme.gridUnit * 2 as its default value? It seems more clear what's the objective. It's hard to grasp that the indent will be added to the margin.

michael-s-molina · 2023-07-19T14:15:22Z

@michael-s-molina I made updates on the addressed ones. Could you help review?

Left 2 comments.

michael-s-molina

LGTM. Thank you for the fix and for addressing all comments!

github-actions · 2023-07-20T18:59:58Z

Ephemeral environment shutdown and build artifacts deleted.

Co-authored-by: Justin Park <justinpark@apache.org> (cherry picked from commit b2831b4)

justinpark requested review from AAfghahi and lyndsiWilliams July 5, 2023 22:58

pull-request-size bot added the size/XL label Jul 5, 2023

justinpark requested review from john-bodley and michael-s-molina July 5, 2023 22:58

michael-s-molina requested review from kasiazjc and eschutho July 6, 2023 11:51

michael-s-molina reviewed Jul 7, 2023

View reviewed changes

superset-frontend/src/components/WarningIconWithTooltip/index.tsx Outdated Show resolved Hide resolved

justinpark force-pushed the fix--add-dataset-left-panel-table-selector-layout branch from 42ef180 to cca1d22 Compare July 13, 2023 22:12

john-bodley added the v3.0 Label added by the release manager to track PRs to be included in the 3.0 branch label Jul 14, 2023

michael-s-molina reviewed Jul 19, 2023

View reviewed changes

Justin Park added 5 commits July 19, 2023 18:19

fix(datasets): Replace left panel layout by TableSelector

fbf9952

spec update for new table selector

b443f8b

Add warning indicator using customLabel renderder

d3af5d6

restore warning icon test spec

f3f14c0

adjust margin right for warning icon

4b0f598

Justin Park and others added 4 commits July 19, 2023 18:19

remove bottom margin on tooltip

478db72

configurable marginRight on warning icon

ac85e6b

remove redundant join/split

3c0c210

replace to marginRight

e5e0358

justinpark force-pushed the fix--add-dataset-left-panel-table-selector-layout branch from cca1d22 to e5e0358 Compare July 20, 2023 01:48

michael-s-molina approved these changes Jul 20, 2023

View reviewed changes

justinpark merged commit b2831b4 into apache:master Jul 20, 2023
26 checks passed

justinpark mentioned this pull request Jul 21, 2023

fix(sqllab): Replace autocomplete logic by a hook #24677

Merged

9 tasks

michael-s-molina pushed a commit that referenced this pull request Jul 26, 2023

fix(datasets): Replace left panel layout by TableSelector (#24599)

d87724a

Co-authored-by: Justin Park <justinpark@apache.org> (cherry picked from commit b2831b4)

justinpark mentioned this pull request Jul 27, 2023

fix(dataset): resizable dataset layout left column #24829

Merged

9 tasks

mistercrunch added 🍒 3.0.0 🍒 3.0.1 🍒 3.0.2 🍒 3.0.3 🍒 3.0.4 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.1.0 labels Mar 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(datasets): Replace left panel layout by TableSelector #24599

fix(datasets): Replace left panel layout by TableSelector #24599

justinpark commented Jul 5, 2023

john-bodley commented Jul 5, 2023 •

edited

Loading

codecov bot commented Jul 6, 2023 •

edited

Loading

eschutho commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

yousoph commented Jul 7, 2023

kasiazjc commented Jul 7, 2023

michael-s-molina commented Jul 7, 2023 •

edited

Loading

justinpark commented Jul 7, 2023

justinpark commented Jul 7, 2023

michael-s-molina Jul 7, 2023

justinpark Jul 13, 2023

michael-s-molina Jul 19, 2023 •

edited

Loading

justinpark Jul 20, 2023

yousoph commented Jul 7, 2023

justinpark commented Jul 11, 2023

justinpark commented Jul 14, 2023

michael-s-molina Jul 19, 2023

michael-s-molina commented Jul 19, 2023

michael-s-molina left a comment

github-actions bot commented Jul 20, 2023

fix(datasets): Replace left panel layout by TableSelector #24599

fix(datasets): Replace left panel layout by TableSelector #24599

Conversation

justinpark commented Jul 5, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

john-bodley commented Jul 5, 2023 • edited Loading

codecov bot commented Jul 6, 2023 • edited Loading

Codecov Report

eschutho commented Jul 6, 2023

github-actions bot commented Jul 6, 2023

yousoph commented Jul 7, 2023

kasiazjc commented Jul 7, 2023

michael-s-molina commented Jul 7, 2023 • edited Loading

justinpark commented Jul 7, 2023

justinpark commented Jul 7, 2023

michael-s-molina Jul 7, 2023

Choose a reason for hiding this comment

justinpark Jul 13, 2023

Choose a reason for hiding this comment

michael-s-molina Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

justinpark Jul 20, 2023

Choose a reason for hiding this comment

yousoph commented Jul 7, 2023

justinpark commented Jul 11, 2023

justinpark commented Jul 14, 2023

michael-s-molina Jul 19, 2023

Choose a reason for hiding this comment

michael-s-molina commented Jul 19, 2023

michael-s-molina left a comment

Choose a reason for hiding this comment

github-actions bot commented Jul 20, 2023

john-bodley commented Jul 5, 2023 •

edited

Loading

codecov bot commented Jul 6, 2023 •

edited

Loading

michael-s-molina commented Jul 7, 2023 •

edited

Loading

michael-s-molina Jul 19, 2023 •

edited

Loading