ktmud commented on a change in pull request #13210:
URL: https://github.com/apache/superset/pull/13210#discussion_r583127546
##########
File path: superset-frontend/src/explore/components/DatasourcePanel.tsx
##########
@@ -28,8 +28,10 @@ import {
import { debounce } from 'lodash';
import { matchSorter, rankings } from 'match-sorter';
import { FAST_DEBOUNCE } from 'src/constants';
-import { ExploreActions } from 'src/explore/actions/exploreActions';
-import Control from 'src/explore/components/Control';
+import { ExploreActions } from '../actions/exploreActions';
Review comment:
```suggestion
import { ExploreActions } from 'src/explore/actions/exploreActions';
```
##########
File path:
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectLabel.tsx
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this 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, { useState } from 'react';
+import { useDrop } from 'react-dnd';
+import { isEmpty } from 'lodash';
+import { t, useTheme } from '@superset-ui/core';
+import { BaseControlConfig, ColumnMeta } from '@superset-ui/chart-controls';
+import ControlHeader from 'src/explore/components/ControlHeader';
+import {
+ AddControlLabel,
+ DndLabelsContainer,
+ HeaderContainer,
+} from 'src/explore/components/OptionControls';
+import {
+ DatasourcePanelDndItem,
+ DatasourcePanelDndType,
+} from 'src/explore/components/DatasourcePanel/types';
+import Icon from 'src/components/Icon';
+import OptionWrapper from './components/OptionWrapper';
+import { OptionSelector } from './utils';
+
+interface LabelProps extends BaseControlConfig {
+ name: string;
+ value: string[] | string | null;
+ onChange: (value: string[] | string | null) => void;
+ options: { string: ColumnMeta };
+}
+
+export default function DndColumnSelectLabel(props: LabelProps) {
+ const theme = useTheme();
+ const { value, options } = props;
+ const optionSelector = new OptionSelector(options, value);
+ const [groupByOptions, setGroupByOptions] = useState<ColumnMeta[]>(
+ optionSelector.groupByOptions,
+ );
+
+ const [{ isOver, canDrop }, datasourcePanelDrop] = useDrop({
+ accept: DatasourcePanelDndType.COLUMN,
+
+ drop: (item: DatasourcePanelDndItem) => {
+ if (!optionSelector.isArray && !isEmpty(optionSelector.groupByOptions)) {
+ optionSelector.replace(0, item.metricOrColumnName);
+ } else {
+ optionSelector.add(item.metricOrColumnName);
+ }
+ setGroupByOptions(optionSelector.groupByOptions);
+ props.onChange(optionSelector.getValues());
+ },
+
+ canDrop: (item: DatasourcePanelDndItem) =>
+ !optionSelector.has(item.metricOrColumnName),
+
+ collect: monitor => ({
+ isOver: monitor.isOver(),
+ canDrop: monitor.canDrop(),
+ type: monitor.getItemType(),
+ }),
+ });
+
+ function onClickClose(index: number) {
+ optionSelector.del(index);
+ setGroupByOptions(optionSelector.groupByOptions);
+ props.onChange(optionSelector.getValues());
+ }
+
+ function onShiftOptions(dragIndex: number, hoverIndex: number) {
+ optionSelector.swap(dragIndex, hoverIndex);
+ setGroupByOptions(optionSelector.groupByOptions);
+ props.onChange(optionSelector.getValues());
+ }
+
+ function placeHolderRenderer() {
Review comment:
Nit:
```suggestion
function renderPlaceholder() {
```
`function placeHolderRenderer` feels like the function will return a
renderer itself.
##########
File path:
superset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
##########
@@ -0,0 +1,86 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this 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 { ColumnMeta } from '@superset-ui/chart-controls';
+
+export class OptionSelector {
Review comment:
I think it's possible to make this generic, too, so you can support
complex options (adhoc metrics, etc) in the list:
```ts
import { ensureIsArray } from '@superset-ui/core';
/**
* Get value from an option array.
*/
type GetValue<T> = (
option: T | string,
index?: number,
array?: (T | string)[],
) => T | string;
/**
* Get option from a value array.
*/
type GetOption<T> = (
value: T | string,
index?: number,
array?: (T | string)[],
) => T;
/**
* Select options from a known list by option value. Ignores invalid options.
*
* @param options - all known options, a dict of value to option.
* @param selected - value of selected options
* @param getValue - how to get value from each option
*/
export class OptionSelector<T> {
options: { [key: string]: T };
/**
* Selected values, always an array.
* If an item is string, then we look it up from options.
*/
selected: (T | string)[];
getValue: GetValue<T>;
getOption: GetOption<T>;
constructor({
options,
selected,
getValue = x => x,
getOption = x => (typeof x === 'string' ? this.options[x] : x),
}: {
options: { [key: string]: T };
selected: (T | string)[] | T | string | null;
getValue?: GetValue<T>;
getOption?: GetOption<T>;
}) {
this.options = options;
this.selected = ensureIsArray(selected)
.map(getValue)
.filter(x => (typeof x === 'string' ? x in options : Boolean(x)));
this.getOption = getOption;
this.getValue = getValue;
}
add(option: T) {
const value = this.getValue(option);
if (typeof value === 'string' || value in this.options) {
this.selected.push(value);
}
}
has(option: T): boolean {
return this.selected.includes(this.getValue(option));
}
replace(idx: number, value: T | string) {
this.selected[idx] = value;
}
del(idx: number) {
this.selected.splice(idx, 1);
}
swap(a: number, b: number) {
[this.selected[a], this.selected[b]] = [this.selected[b],
this.selected[a]];
}
/**
* Return selected options from value.
*/
selectedOptions(): T[] {
return this.selected.map(this.getOption);
}
}
// since `isMulti` should be managed in the control state:
isMulti ? optionSelector.selectedOptions() :
optionSelector.selectedOptions()[0];
```
Just a rough prototype but you get the idea.
##########
File path:
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectLabel.tsx
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this 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, { useState } from 'react';
+import { useDrop } from 'react-dnd';
+import { isEmpty } from 'lodash';
+import { t, useTheme } from '@superset-ui/core';
+import { BaseControlConfig, ColumnMeta } from '@superset-ui/chart-controls';
+import ControlHeader from 'src/explore/components/ControlHeader';
+import {
+ AddControlLabel,
+ DndLabelsContainer,
+ HeaderContainer,
+} from 'src/explore/components/OptionControls';
+import {
+ DatasourcePanelDndItem,
+ DatasourcePanelDndType,
+} from 'src/explore/components/DatasourcePanel/types';
+import Icon from 'src/components/Icon';
+import OptionWrapper from './components/OptionWrapper';
+import { OptionSelector } from './utils';
+
+interface LabelProps extends BaseControlConfig {
+ name: string;
+ value: string[] | string | null;
+ onChange: (value: string[] | string | null) => void;
+ options: { string: ColumnMeta };
+}
+
+export default function DndColumnSelectLabel(props: LabelProps) {
+ const theme = useTheme();
+ const { value, options } = props;
+ const optionSelector = new OptionSelector(options, value);
+ const [groupByOptions, setGroupByOptions] = useState<ColumnMeta[]>(
+ optionSelector.groupByOptions,
+ );
+
+ const [{ isOver, canDrop }, datasourcePanelDrop] = useDrop({
+ accept: DatasourcePanelDndType.COLUMN,
+
+ drop: (item: DatasourcePanelDndItem) => {
+ if (!optionSelector.isArray && !isEmpty(optionSelector.groupByOptions)) {
+ optionSelector.replace(0, item.metricOrColumnName);
+ } else {
+ optionSelector.add(item.metricOrColumnName);
+ }
+ setGroupByOptions(optionSelector.groupByOptions);
+ props.onChange(optionSelector.getValues());
+ },
+
+ canDrop: (item: DatasourcePanelDndItem) =>
+ !optionSelector.has(item.metricOrColumnName),
Review comment:
Currently only columns have a `canDrop` indicator when dropping to a
column select control, if you drag a metric, nothing will happen. It would be
nice to allow drag & drop anything to any drop area while showing a useful
`can't drop` warning when appropriate.
E.g.
<img width="1184" alt="Screen Shot 2021-02-25 at 10 32 42 AM"
src="https://user-images.githubusercontent.com/335541/109213860-f7ef7600-7765-11eb-93ad-afdf068a05af.png";>
Example error messages:
- Column already added
- Cannot group by a metric
##########
File path:
superset-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectLabel.tsx
##########
@@ -0,0 +1,119 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this 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, { useState } from 'react';
+import { useDrop } from 'react-dnd';
+import { isEmpty } from 'lodash';
+import { t, useTheme } from '@superset-ui/core';
+import { BaseControlConfig, ColumnMeta } from '@superset-ui/chart-controls';
+import ControlHeader from 'src/explore/components/ControlHeader';
+import {
+ AddControlLabel,
+ DndLabelsContainer,
+ HeaderContainer,
+} from 'src/explore/components/OptionControls';
+import {
+ DatasourcePanelDndItem,
+ DatasourcePanelDndType,
+} from 'src/explore/components/DatasourcePanel/types';
+import Icon from 'src/components/Icon';
+import OptionWrapper from './components/OptionWrapper';
+import { OptionSelector } from './utils';
+
+interface LabelProps extends BaseControlConfig {
+ name: string;
+ value: string[] | string | null;
Review comment:
I think you may still need a `isMulti` flag because `null` could
represents empty select for multiple selects as well.
##########
File path:
superset-frontend/src/explore/components/controls/DndColumnSelectControl/types.ts
##########
@@ -0,0 +1,34 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this 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 { ColumnMeta } from '@superset-ui/chart-controls';
+
+export interface OptionProps {
+ column: ColumnMeta;
+ index: number;
+ clickClose: (index: number) => void;
+ onShiftOptions: (dragIndex: number, hoverIndex: number) => void;
+ withCaret?: boolean;
+}
+
+export const GroupByItemType = 'groupByItem';
Review comment:
One way to make this more generic:
```suggestion
/**
* 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',
}
```
- If a `DndItemType.metric` or `DndItemType.metricOption` is dropped to a
group by (column select) control, reject and show a warning;
- if a `DndItemType.column` or `columnOption` is dropped to metric control,
create an adhoc metric using this column.
- If a `metric`, `metricOption`, `column`, or `columnOption` is dragged to
the adhoc filter control or sort by control, create a new filter using the
metric or column.
##########
File path: superset-frontend/src/explore/components/DatasourcePanel/types.ts
##########
@@ -0,0 +1,30 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this 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.
+ */
+export enum DatasourcePanelDndType {
+ // todo: The new `metric` conflicts with the existing metric type
+ METRIC = 'datasource-panel-metric',
+ COLUMN = 'column',
+}
+
+export interface DatasourcePanelDndItem {
+ metricOrColumnName: string;
+ type:
+ | typeof DatasourcePanelDndType.METRIC
+ | typeof DatasourcePanelDndType.COLUMN;
Review comment:
This should also work:
```suggestion
type: DatasourcePanelDndType;
```
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
![https://user-images.githubusercontent.com/335541/109213860-f7ef7600-7765-11eb-93ad-afdf068a05af.png"](https://user-images.githubusercontent.com/335541/109213860-f7ef7600-7765-11eb-93ad-afdf068a05af.png"){kind=link}