feat: Implement drag and drop columns for filters #13340

kgabryje · 2021-02-25T16:15:17Z

SUMMARY

The goal was to implement creating adhoc filters by dragging columns from datasource panel to filters control. Also, I attempted to make Yongjie's (#13210) implementation more generic, so that we can reuse components with filters and, in the future, with metrics controls.

CC: @villebro @zhaoyongjie @ktmud @junlincc

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-02-25.at.16.46.43.mov

TEST PLAN

Pull apache-superset/superset-ui#988, link chart-controls to Superset, set ENABLE_EXPLORE_DRAG_AND_DROP feature flag.

ADDITIONAL INFORMATION

ktmud · 2021-02-25T19:38:04Z

In current state it's difficult to review, so I suggest delaying reviews until the original PR is merged.

Can we create a feature branch? Not sure if this goes against the Apache way, but maybe you can also create a PR to @zhaoyongjie 's fork/branch and tag us for review over there.

kgabryje · 2021-02-26T11:24:56Z

you can also create a PR to @zhaoyongjie 's fork/branch and tag us for review over there.

Creating a PR to an original branch sounds good, I'll do that next time. This branch is already rebased on master as Yongjie's PR was merged, so it's ready for review.

zhaoyongjie · 2021-02-26T12:48:49Z

I personally suggest not to make generic components now. The main reason is that these components are completely inconsistent in function, and we should not do such maintenance that increases complexity.

For dimension(group by) selector

columns (can shift between)
Level columns(AKA hierarchy) (there may be folding between levels, can not shift between)
columns set(such as generate case...when... clause)

For filter selector, Superset is now just a simple dimension filter

If we want to drag saved_metric to generate a having clause, we need to enable metic drop to filter
columns, or top level of hierarchy
Drag into different types of columns should have different filter

For metric selector

columns ( generate ad-hoc metric)
saved_metric

suggestions are welcome

ktmud · 2021-02-26T19:14:24Z

There're definitely certain things you can abstract away for all these. The basic interactions are the same, it's just what happens after you drop is different. It should be possible to let each different drop item and target extend from the same base class or React hook.

At the very least, DnD item types should be managed in a global registry (eg. as an Enum).

ktmud · 2021-03-01T20:08:37Z

superset-frontend/src/explore/components/controls/DndColumnSelectControl/types.ts

+import { DatasourcePanelDndItem } from '../../DatasourcePanel/types';
+
+export const GroupByItemType = 'groupByItem';
+export const FilterItemType = 'filterItemType';


https://github.com/apache/superset/pull/13210/files#r583265905

I'd imagine you will be adding more types in the future, so it could make sense to make this an enum.

ktmud · 2021-03-01T20:09:57Z

...rset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts

@@ -19,26 +19,26 @@
 import { ColumnMeta } from '@superset-ui/chart-controls';

 export class OptionSelector {


https://github.com/apache/superset/pull/13210/files#r583272323

Here's one way to make this generic so that adhoc columns/metrics can also be selected. But of course you can also make sure each adhoc item have a unique id/label and use is as the option key.

kgabryje · 2021-03-03T15:36:37Z

Update:
New feature - drag and drop saved metrics from Datasource Panel and adhoc metrics to filters box to quickly create a filter based on a metric.
Fix typescript errors.
@ktmud thank you for your suggestions - I applied your solution to use enum for dnd types; making OptionSelector more generic is in progress
To test the new changes, please pull and link apache-superset/superset-ui#988
CC @villebro @zhaoyongjie

villebro

A few non-blocking comments. Tested all different scenarios I could think of (dragging column, saved metric, calculated column and adhoc metric into the filter control), and all worked beautifully. Code looks super solid but due to the size of the PR something might have gone unnoticed. However, since this is behind a FF and I wasn't able to notice any problems with the FF turned off, I think this is good to go.

villebro · 2021-03-04T19:17:49Z

superset-frontend/src/explore/components/DndItemType.ts

+export enum DndItemType {
+  // an existing column in table
+  column = 'column',
+  // a selected column option in ColumnSelectControl
+  columnOption = 'columnOption',
+  // an adhoc column option in ColumnSelectControl
+  adhocColumnOption = 'adhocColumn',
+
+  // a saved metric
+  metric = 'metric',
+  // a selected saved metric in MetricsControl
+  metricOption = 'metricOption',
+  // an adhoc metric option in MetricsControl
+  adhocMetricOption = 'adhocMetric',
+
+  // an adhoc filter option
+  filterOption = 'filterOption',
+}


nit: I believe the common convention is to use PascalCase for enum names:
https://www.typescriptlang.org/docs/handbook/enums.html

villebro · 2021-03-04T19:26:13Z

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

+import { DndItemType } from '../../DndItemType';
+
+const isDictionaryForAdhocFilter = (value: FilterOptionValueType) =>
+  value && !(value instanceof AdhocFilter) && value.expressionType;


I assume checking for value here isn't necessary as value is always an object (=always truthy)?

I removed value and used value?.expressionType instead, just in case it was null or undefined

villebro · 2021-03-07T09:54:02Z

Merging this to unblock other PRs requiring new features from superset-ui (let's address any potential review comments in a follow-up PR)

* Implement DnD feature for filters * minor refactor * Fix types * Fix undefined error * Refactor * Fix ts errors * Fix conflicting dnd types * Bump superset-ui packages * Change DndItemType case to PascalCase * Remove redundant null check * Fix * Fix csrf mock api call

pull-request-size bot added the size/XXL label Feb 25, 2021

villebro self-requested a review February 25, 2021 16:53

junlincc added the explore:control Related to the controls panel of Explore label Feb 25, 2021

junlincc added explore:drag&drop Related to drag&drop in Explore and removed explore:control Related to the controls panel of Explore labels Feb 25, 2021

kgabryje force-pushed the feat/dnd-filters branch from dc34ade to c215550 Compare February 26, 2021 11:21

pull-request-size bot added size/XL and removed size/XXL labels Feb 26, 2021

kgabryje force-pushed the feat/dnd-filters branch from c215550 to ad66337 Compare March 1, 2021 16:48

ktmud reviewed Mar 1, 2021

View reviewed changes

kgabryje force-pushed the feat/dnd-filters branch from ad66337 to 2df1f79 Compare March 3, 2021 15:25

kgabryje mentioned this pull request Mar 3, 2021

fix: rename DnD types to match component names in Superset apache-superset/superset-ui#988

Merged

kgabryje added 7 commits March 4, 2021 16:49

Implement DnD feature for filters

6d74a2a

minor refactor

ac3d7dc

Fix types

b3bdb81

Fix undefined error

6448fe1

Refactor

b8abf78

Fix ts errors

1e1b802

Fix conflicting dnd types

0bd133e

kgabryje force-pushed the feat/dnd-filters branch from 9da4682 to 0bd133e Compare March 4, 2021 16:02

Bump superset-ui packages

4a842e8

villebro approved these changes Mar 4, 2021

View reviewed changes

villebro requested a review from zhaoyongjie March 5, 2021 07:23

kgabryje added 2 commits March 5, 2021 09:40

Change DndItemType case to PascalCase

ce776bb

Remove redundant null check

852cad4

pull-request-size bot added size/XXL and removed size/XL labels Mar 5, 2021

Fix

9aaa98b

pull-request-size bot added size/XL and removed size/XXL labels Mar 5, 2021

Fix csrf mock api call

b05b622

villebro merged commit 7b370e6 into apache:master Mar 7, 2021

mihir174 mentioned this pull request Apr 14, 2021

[SIP-62] Explore control panel Drag & Drop Interaction patterns and new design #14114

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Implement drag and drop columns for filters #13340

feat: Implement drag and drop columns for filters #13340

kgabryje commented Feb 25, 2021 •

edited

Loading

ktmud commented Feb 25, 2021 •

edited

Loading

kgabryje commented Feb 26, 2021

zhaoyongjie commented Feb 26, 2021 •

edited

Loading

ktmud commented Feb 26, 2021

ktmud Mar 1, 2021

ktmud Mar 1, 2021

kgabryje commented Mar 3, 2021 •

edited

Loading

villebro left a comment

villebro Mar 4, 2021

kgabryje Mar 5, 2021

villebro Mar 4, 2021

kgabryje Mar 5, 2021

villebro commented Mar 7, 2021

		@@ -19,26 +19,26 @@
		import { ColumnMeta } from '@superset-ui/chart-controls';

		export class OptionSelector {

feat: Implement drag and drop columns for filters #13340

feat: Implement drag and drop columns for filters #13340

Conversation

kgabryje commented Feb 25, 2021 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

ktmud commented Feb 25, 2021 • edited Loading

kgabryje commented Feb 26, 2021

zhaoyongjie commented Feb 26, 2021 • edited Loading

For dimension(group by) selector

For filter selector, Superset is now just a simple dimension filter

For metric selector

ktmud commented Feb 26, 2021

ktmud Mar 1, 2021

Choose a reason for hiding this comment

ktmud Mar 1, 2021

Choose a reason for hiding this comment

kgabryje commented Mar 3, 2021 • edited Loading

villebro left a comment

Choose a reason for hiding this comment

villebro Mar 4, 2021

Choose a reason for hiding this comment

kgabryje Mar 5, 2021

Choose a reason for hiding this comment

villebro Mar 4, 2021

Choose a reason for hiding this comment

kgabryje Mar 5, 2021

Choose a reason for hiding this comment

villebro commented Mar 7, 2021

kgabryje commented Feb 25, 2021 •

edited

Loading

ktmud commented Feb 25, 2021 •

edited

Loading

zhaoyongjie commented Feb 26, 2021 •

edited

Loading

kgabryje commented Mar 3, 2021 •

edited

Loading