You are viewing a plain text version of this content. The canonical link for it is here.

Posted to notifications@superset.apache.org by GitBox <gi...@apache.org> on 2021/02/25 16:15:29 UTC

[GitHub] [superset] kgabryje opened a new pull request #13340: feat: Implement drag and drop columns for filters

kgabryje opened a new pull request #13340:
URL: https://github.com/apache/superset/pull/13340


   ### SUMMARY
   This PR is based on https://github.com/apache/superset/pull/13210 by @zhaoyongjie. Once https://github.com/apache/superset/pull/13210 is merged, this PR needs to get rebased onto master.
   
   The goal was to implement creating adhoc filters by dragging columns from datasource panel to filters control. Also, I  attempted to make Yongjie's 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
   https://user-images.githubusercontent.com/15073128/109182131-f9806480-778c-11eb-97bc-fef2b579bb2f.mov
   
   ### TEST PLAN
   Pull https://github.com/apache-superset/superset-ui/pull/978, link chart-controls to Superset, set `ENABLE_EXPLORE_DRAG_AND_DROP` feature flag.
   
   ### ADDITIONAL INFORMATION
   <!--- Check any relevant boxes with "x" -->
   <!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
   - [ ] Has associated issue:
   - [ ] Changes UI
   - [ ] Requires DB Migration.
   - [ ] Confirm DB Migration upgrade and downgrade tested.
   - [ ] Introduces new feature or API
   - [ ] Removes existing feature or API
   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] ktmud commented on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

ktmud commented on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-786841583


   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).


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie edited a comment on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

zhaoyongjie edited a comment on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-786627899

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
1. columns (can shift between)
2. Level columns(AKA hierarchy) (there may be folding between levels, can not shift between)
3. [columns set](https://help.tableau.com/current/pro/desktop/en-us/sortgroup_sets_create.htm)(such as generate `case...when...` clause)

#### For filter selector, Superset is now just a simple dimension filter
1. If we want to drag saved_metric to generate a having clause, we need to enable metic drop to filter
2. columns, or top level of hierarchy
3. Drag into different types of columns should have different filter

#### For metric selector
1. columns ( generate ad-hoc metric)
2. saved_metric

Maybe I have some inconsiderate for consider.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] ktmud commented on a change in pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

ktmud commented on a change in pull request #13340:
URL: https://github.com/apache/superset/pull/13340#discussion_r585015220



##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/types.ts
##########
@@ -16,19 +16,60 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-import { ColumnMeta } from '@superset-ui/chart-controls';
+import { ReactNode } from 'react';
+import { AdhocFilter } from '@superset-ui/core';
+import {
+  BaseControlConfig,
+  ColumnMeta,
+  Metric,
+} from '@superset-ui/chart-controls';
+import { DatasourcePanelDndItem } from '../../DatasourcePanel/types';
+
+export const GroupByItemType = 'groupByItem';
+export const FilterItemType = 'filterItemType';

Review comment:
       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.

##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -19,26 +19,26 @@
 import { ColumnMeta } from '@superset-ui/chart-controls';
 
 export class OptionSelector {

Review comment:
       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.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on a change in pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

villebro commented on a change in pull request #13340:
URL: https://github.com/apache/superset/pull/13340#discussion_r587756311



##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx
##########
@@ -0,0 +1,334 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership.  The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useMemo, useState } from 'react';
+import { logging, SupersetClient } from '@superset-ui/core';
+import { ColumnMeta, Metric } from '@superset-ui/chart-controls';
+import { Tooltip } from 'src/common/components/Tooltip';
+import { OPERATORS } from 'src/explore/constants';
+import { OptionSortType } from 'src/explore/types';
+import { DndFilterSelectProps, FilterOptionValueType } from './types';
+import AdhocFilterPopoverTrigger from '../FilterControl/AdhocFilterPopoverTrigger';
+import OptionWrapper from './components/OptionWrapper';
+import DndSelectLabel from './DndSelectLabel';
+import AdhocFilter, {
+  CLAUSES,
+  EXPRESSION_TYPES,
+} from '../FilterControl/AdhocFilter';
+import AdhocMetric from '../MetricControl/AdhocMetric';
+import {
+  DatasourcePanelDndItem,
+  DndItemValue,
+} from '../../DatasourcePanel/types';
+import { DndItemType } from '../../DndItemType';
+
+const isDictionaryForAdhocFilter = (value: FilterOptionValueType) =>
+  value && !(value instanceof AdhocFilter) && value.expressionType;

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

##########
File path: superset-frontend/src/explore/components/DndItemType.ts
##########
@@ -16,7 +16,25 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-export const OPTION_TYPES = {
-  metric: 'metric',
-  filter: 'filter',
-};
+
+/**
+ * All possible draggable items for the chart controls.
+ */
+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',
+}

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




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] ktmud commented on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

ktmud commented on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-786151488


   > 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 his fork/branch and tag us for review over there.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] kgabryje edited a comment on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

kgabryje edited a comment on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-789804263


   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 https://github.com/apache-superset/superset-ui/pull/988
   CC @villebro @zhaoyongjie 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] kgabryje commented on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

kgabryje commented on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-789804263


   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
   CC @villebro @zhaoyongjie 


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] kgabryje commented on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

kgabryje commented on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-786588719


   > 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.


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro commented on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

villebro commented on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-792248694


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


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie commented on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

zhaoyongjie commented on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-786627899

#### For metric selector
1. columns ( generate ad-hoc metric)
2. saved_metric

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [superset] ktmud edited a comment on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

ktmud edited a comment on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-786151488






----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] villebro merged pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

villebro merged pull request #13340:
URL: https://github.com/apache/superset/pull/13340


   


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org

[GitHub] [superset] zhaoyongjie edited a comment on pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

zhaoyongjie edited a comment on pull request #13340:
URL: https://github.com/apache/superset/pull/13340#issuecomment-786627899

#### For metric selector
1. columns ( generate ad-hoc metric)
2. saved_metric

suggestions are welcome

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [superset] kgabryje commented on a change in pull request #13340: feat: Implement drag and drop columns for filters

Posted by GitBox <gi...@apache.org>.

kgabryje commented on a change in pull request #13340:
URL: https://github.com/apache/superset/pull/13340#discussion_r588123050



##########
File path: superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndFilterSelect.tsx
##########
@@ -0,0 +1,334 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with work for additional information
+ * regarding copyright ownership.  The ASF licenses file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import React, { useEffect, useMemo, useState } from 'react';
+import { logging, SupersetClient } from '@superset-ui/core';
+import { ColumnMeta, Metric } from '@superset-ui/chart-controls';
+import { Tooltip } from 'src/common/components/Tooltip';
+import { OPERATORS } from 'src/explore/constants';
+import { OptionSortType } from 'src/explore/types';
+import { DndFilterSelectProps, FilterOptionValueType } from './types';
+import AdhocFilterPopoverTrigger from '../FilterControl/AdhocFilterPopoverTrigger';
+import OptionWrapper from './components/OptionWrapper';
+import DndSelectLabel from './DndSelectLabel';
+import AdhocFilter, {
+  CLAUSES,
+  EXPRESSION_TYPES,
+} from '../FilterControl/AdhocFilter';
+import AdhocMetric from '../MetricControl/AdhocMetric';
+import {
+  DatasourcePanelDndItem,
+  DndItemValue,
+} from '../../DatasourcePanel/types';
+import { DndItemType } from '../../DndItemType';
+
+const isDictionaryForAdhocFilter = (value: FilterOptionValueType) =>
+  value && !(value instanceof AdhocFilter) && value.expressionType;

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

##########
File path: superset-frontend/src/explore/components/DndItemType.ts
##########
@@ -16,7 +16,25 @@
  * specific language governing permissions and limitations
  * under the License.
  */
-export const OPTION_TYPES = {
-  metric: 'metric',
-  filter: 'filter',
-};
+
+/**
+ * All possible draggable items for the chart controls.
+ */
+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',
+}

Review comment:
       👍 




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org



---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org