refactor(explore): improve typing for Dnd controls #16362

ktmud · 2021-08-19T19:14:44Z

SUMMARY

Improve typing for Explore Dnd controls. Some principles:

React component props always go with the component itself---unless props are shared between multiple components
Reuse ControlProps to get a more complete shared props propagated from parent.

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

ktmud · 2021-08-19T19:23:43Z

[HOLD] The UX for overriding single select MetricControl is quite confusing. While Drag & Drop, There is no clear indication that a control will only accept one metric.

kgabryje · 2021-08-20T13:06:17Z

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

-      });
-    }, FAST_DEBOUNCE),
-    [columns, metrics],
+  const search = debounce(


Is there a reason why you changed the order here? According to https://dmitripavlutin.com/react-throttle-debounce/ useCallback should go first (although it seems that what we had here wasn't fully correct as we should wrap debounce function in useMemo...)

ESLint react-hook plugin will complain not being able to detect exhaustive deps and suggests making the callback inline.

I've just verified - simply replacing useCallback with useMemo solves the eslint problem

const search = useMemo( () => debounce((value: string) => { ...

Cool. That should work!

kgabryje · 2021-08-20T13:08:44Z

[HOLD] The UX for overriding single select MetricControl is quite confusing. While Drag & Drop, There is no clear indication that a control will only accept one metric.

We indicate that a control accepts only 1 value by not displaying ghost button when a value is added. However, I do agree that we don't communicate clearly that dropping another value will replace the first one...

junlincc · 2021-08-20T16:43:49Z

thanks this is awesome! @ktmud do you mind splitting this pr into 2 one for improving typing one for logging?

ktmud · 2021-08-20T17:54:27Z

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx

+  columns: ColumnMeta[];
+  datasourceType?: DatasourceType;
+};
+
 export const DndMetricSelect = (props: any) => {


TODO: get rid of this any. Currently DndMetricSelectProps will conflict with too many things within this function. This file needs a bigger overhaul.

ktmud · 2021-08-20T17:55:34Z

@kgabryje do you mind taking another look?

codecov · 2021-08-20T18:21:35Z

Codecov Report

Merging #16362 (7c45fb3) into master (518c3c9) will increase coverage by 0.01%.
The diff coverage is 68.00%.

❗ Current head 7c45fb3 differs from pull request most recent head 6e8ff55. Consider uploading reports for the commit 6e8ff55 to get more accurate results

@@            Coverage Diff             @@
##           master   #16362      +/-   ##
==========================================
+ Coverage   76.64%   76.66%   +0.01%     
==========================================
  Files        1000     1000              
  Lines       53486    53488       +2     
  Branches     6815     6816       +1     
==========================================
+ Hits        40995    41004       +9     
+ Misses      12255    12248       -7     
  Partials      236      236

Flag	Coverage Δ
javascript	`70.91% <68.00%> (+0.02%)`	⬆️

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

Impacted Files	Coverage Δ
...perset-frontend/src/explore/components/Control.tsx	`76.47% <ø> (ø)`
...-frontend/src/explore/components/ControlHeader.jsx	`86.20% <ø> (ø)`
superset-frontend/src/views/CRUD/types.ts	`100.00% <ø> (ø)`
...ontrols/DndColumnSelectControl/DndMetricSelect.tsx	`40.88% <20.00%> (-0.53%)`	⬇️
...d/src/explore/components/DatasourcePanel/index.tsx	`73.95% <69.23%> (ø)`
...ontrols/DndColumnSelectControl/DndColumnSelect.tsx	`47.56% <100.00%> (ø)`
...ontrols/DndColumnSelectControl/DndFilterSelect.tsx	`51.33% <100.00%> (+6.00%)`	⬆️
...controls/DndColumnSelectControl/DndSelectLabel.tsx	`77.27% <100.00%> (ø)`
...ols/DndColumnSelectControl/utils/optionSelector.ts	`46.15% <100.00%> (ø)`

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 518c3c9...6e8ff55. Read the comment docs.

kgabryje · 2021-08-26T07:49:34Z

superset-frontend/src/explore/components/Control.tsx

@@ -18,6 +18,7 @@
 */
 import React, { ReactNode, useCallback, useState } from 'react';
 import { ControlType } from '@superset-ui/chart-controls';
+import { ControlComponentProps as BaseControlComponentProps } from '@superset-ui/chart-controls/lib/shared-controls/components/types';


Non-blocking - should we export that from chart-controls in the next release of superset-ui, so that we can import from '@superset-ui/chart-controls'?

Agreed. I'll do that the next time I touch superset-ui.

kgabryje

That's a HUGE improvement, thanks!

* upstream/master: (25 commits) chore(ci): bump pylint to 2.10.2 (apache#16463) fix: prevent page crash when chart can't render (apache#16464) chore: fixed slack invite link (apache#16466) fix(native-filters): handle null values in value filter (apache#16460) feat: add function list to auto-complete to Clickhouse datasource (apache#16234) refactor(explore): improve typing for Dnd controls (apache#16362) fix(explore): update overwrite button on perm change (apache#16437) feat: Draggable and Resizable Modal (apache#16394) refactor: sql_json view endpoint (apache#16441) fix(dashboard): undo and redo buttons weird alignment (apache#16417) fix: setupPlugin in chart list page (apache#16413) fix: Disable Slack notification method if no api token (apache#16367) feat: add Shillelagh DB engine spec (apache#16416) fix: copy to Clipboard order (apache#16299) docs: make FEATURE_FLAGS.md reference a link (apache#16415) chore(viz): bump superset-ui to 0.17.87 (apache#16420) feat: add activate command (apache#16404) Revert "fix(explore): let admin overwrite slice (apache#16290)" (apache#16408) fix(explore): retain chart ownership on query context update (apache#16419) chore: Removes the TODOs and uses the default page size (apache#16422) ...

michellethomas · 2021-08-26T18:41:07Z

superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndMetricSelect.tsx

-      const newValue = props.multi ? [...value, newMetric] : [newMetric];
+      const newValue = props.multi
+        ? [...value, newMetric.metric_name]
+        : [newMetric.metric_name];


@ktmud I believe this introduced a bug with dragging columns into the metrics section and creating a SIMPLE expression for the column. When you drag a column into the metrics section the data looks like the data listed below and does not have a metric_name. This leads to an undefined option being returned and later errors:

aggregate: "COUNT_DISTINCT" column: {column_name: "target"} expressionType: "SIMPLE" hasCustomLabel: false isNew: false label: "COUNT_DISTINCT(target)" optionName: "metric_1vo4t2m2tjl_0nhrga77x9pf" sqlExpression: null

Got it. Thanks for the testing and pointing this out!

pull-request-size bot added the size/XL label Aug 19, 2021

kgabryje reviewed Aug 20, 2021

View reviewed changes

ktmud force-pushed the log-dnd-usage branch from 00701c5 to df59ec9 Compare August 20, 2021 17:44

pull-request-size bot added size/L and removed size/XL labels Aug 20, 2021

ktmud changed the title ~~feat(explore): improve typing for Dnd controls and add logging~~ refactor(explore): improve typing for Dnd controls Aug 20, 2021

refactor(explore): improve typing for Dnd controls

6e8ff55

ktmud force-pushed the log-dnd-usage branch from df59ec9 to 6e8ff55 Compare August 20, 2021 17:50

ktmud commented Aug 20, 2021

View reviewed changes

ktmud marked this pull request as ready for review August 20, 2021 17:54

junlincc added the risk:refactor High risk as it involves large refactoring work label Aug 21, 2021

ktmud requested a review from kgabryje August 24, 2021 20:25

kgabryje reviewed Aug 26, 2021

View reviewed changes

kgabryje approved these changes Aug 26, 2021

View reviewed changes

ktmud merged commit ec08750 into apache:master Aug 26, 2021

michellethomas reviewed Aug 26, 2021

View reviewed changes

This was referenced Aug 26, 2021

[dragndrop]unexpected error in metric - pivot table #16471

Closed

[dragndrop] Feature request: Clarify drop and replace for single metric control #16476

Closed

ktmud mentioned this pull request Aug 26, 2021

fix(explore): JS error for creating new metrics from columns #16477

Merged

9 tasks

ktmud deleted the log-dnd-usage branch August 26, 2021 22:29

opus-42 pushed a commit to opus-42/incubator-superset that referenced this pull request Nov 14, 2021

refactor(explore): improve typing for Dnd controls (apache#16362)

f89690c

QAlexBall pushed a commit to QAlexBall/superset that referenced this pull request Dec 28, 2021

refactor(explore): improve typing for Dnd controls (apache#16362)

5bb44bb

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(explore): improve typing for Dnd controls #16362

refactor(explore): improve typing for Dnd controls #16362

ktmud commented Aug 19, 2021 •

edited

Loading

ktmud commented Aug 19, 2021 •

edited

Loading

kgabryje Aug 20, 2021

ktmud Aug 20, 2021

kgabryje Aug 20, 2021

ktmud Aug 20, 2021

kgabryje commented Aug 20, 2021

junlincc commented Aug 20, 2021

ktmud Aug 20, 2021

ktmud commented Aug 20, 2021

codecov bot commented Aug 20, 2021

kgabryje Aug 26, 2021

ktmud Aug 26, 2021

kgabryje left a comment

michellethomas Aug 26, 2021

ktmud Aug 26, 2021

refactor(explore): improve typing for Dnd controls #16362

refactor(explore): improve typing for Dnd controls #16362

Conversation

ktmud commented Aug 19, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

ktmud commented Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje commented Aug 20, 2021

junlincc commented Aug 20, 2021

Choose a reason for hiding this comment

ktmud commented Aug 20, 2021

codecov bot commented Aug 20, 2021

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgabryje left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ktmud commented Aug 19, 2021 •

edited

Loading

ktmud commented Aug 19, 2021 •

edited

Loading