-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(explore): ColumnSelectControl with drag-and-drop #13210
Conversation
Codecov Report
@@ Coverage Diff @@
## master #13210 +/- ##
==========================================
+ Coverage 77.18% 80.85% +3.66%
==========================================
Files 881 300 -581
Lines 45499 24420 -21079
Branches 5447 0 -5447
==========================================
- Hits 35119 19745 -15374
+ Misses 10256 4675 -5581
+ Partials 124 0 -124
Flags with carried forward coverage won't be shown. Click here to find out more. Continue to review full report at Codecov.
|
@zhaoyongjie Yongjie, thanks for the POC! super excited about it. can you add manual text plan and instruction of turning on the feature flag in description? thanks! |
superset-frontend/spec/javascripts/explore/components/DatasourcePanel_spec.jsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/DatasourcePanel/DatasourcePanelDragWrapper.tsx
Outdated
Show resolved
Hide resolved
...set-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectLabel.tsx
Outdated
Show resolved
Hide resolved
...set-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectLabel.tsx
Outdated
Show resolved
Hide resolved
superset-frontend/src/explore/components/controls/DndColumnSelectControl/components/Option.tsx
Outdated
Show resolved
Hide resolved
metricOrColumnName={m.metric_name} | ||
type={DatasourcePanelDndType.METRIC} | ||
> | ||
<MetricOption metric={m} showType /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about using a cursor: move
instead of default here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry, what is cursor: move
on this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, I will add indicator style on this component and drop component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The style pointer: move
on the datasource panel looks strange. the UI behavior we can ask @mihir174
move.mp4
|
||
function placeHolderRenderer() { | ||
return ( | ||
<AddControlLabel cancelHover> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to allow users to add groupby "normally" - by selecting from options instead of drag and dropping? CC @junlincc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess, select control
and drag and drag control
need choose one of two.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, I suppose it's a product decision and we can wait for Junlin's input, but I'd go for allowing users to choose the way they want to interact with our control panel. Removing normal select in favour of dnd feels like a step back to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code logic of the selector is already very complicated, and it is difficult to add complex logic (such as hierarchical). I suggest referring to modern BI design.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kgabryje thanks for suggesting!
Yes, users may take awhile to get used to the new behavior, but if we keep both select and dnd, my concern is
it will increase the complexity of the component
- we have two sets of each control to maintain, also if one breaks, it will likely affect the other as well?
- more overhead if we wanna introduce new feature, for example in metrics.
We could keep both until we officially turn feature flag off though and run some usability tests to compare both solutions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(regarding dnd only vs also supporting traditional clicking the control) I personally feel it's better to offer the users the option of using the traditional approach, especially as long there is a sensible control which you can click. While e.g. Tableau AFAIK only supports dnd interaction, in that UI it makes more sense, but in Superset I feel it's still a perfectly valid path to adding filters/metrics/columns.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This contentious point is almost the same as whether we should have "+" AND "ghost" button for click-to-select, and should we still support FilterBox after having native filter.
Most of us believe that the "ghost" button is more intuitive, while many from the community still prefer the existing way for various reasons. In my roadmap google sheet, I stated that the biggest drives of implementing this feature are competitive, marketability, UX improvement.
Competitive - Tableau only offers DND, no SELECT. The question I have for ya'll is that how can we find out whether offering SELECT as well ADD any value, and what that would be? We will find out later during usability or A/B testing. This should be a decision supported by data, let's not make decision for the users.
Marketability - Self explanatory, this driver doesn't affect final decision much
User Experience - keep both under FF, run test, if having both significantly impact UX in a positive way, why not keeping both?
Cost - this is more a stakeholder question. do we want to spend 2x time to maintain 2 sets of feature that achieve basically the same goal, or do we want to spend our engineering/product/QA effort on a set of new feature that brings more value and offers higher ROI?
Let's hand over the ball to our designer @mihir174, I'm sure he will take all our feedback and come up with the best solution forusers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand this, and as long as this is behind a FF, I agree we can live with some temporary glitches in the feature. Some thoughts that might be relevant for Mihir when considering options:
- We currently only support max 50 columns in the column list. While it's possible to filter the list, it is currently not possible to browse the full list like it is in the current select control.
- If the filter and metric DND controls will support both DND and the click, it would make sense to also support select on the column control (alternatively it might make sense to disable non-DND interaction with the filter and metric controls).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently only support max 50 columns in the column list. While it's possible to filter the list, it is currently not possible to browse the full list like it is in the current select control.
the panel search bar is very robust which supports key word search in column & metric names, label/verbose name, description, and even warning, information. when # of column reach 50, i doubt scrolling through the list is the primary way to look for the objects.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@junlincc Trust me, most users are lazy and would not want to type. Browsing the list to add columns is probably a much more common use case than you thought. For example, in Airbnb, in one of the datasources, there are many metrics with "nights_booked" in the name, when users want to find a specific metric related to "nights_booked" but couldn't remember the exact name, even after they typed "nights_booked", they may still need to browse through more than 50 options.
Re: offering DnD only or both interactions:
I think one of the biggest advantage of Superset is the ability to create adhoc metric very easily. I'm struggling to imagine how that workflow of conveniently creating adhoc metrics/columns would look like without a popover. If we are going to build a popover anyway, why not keep both DnD and click? With proper design and abstraction, the code complexity should be manageable.
Re: code maintenance:
That said, current code indeed is very complex, and is not even in TypeScript, so I'm in support of writing the new control bit by bit from scratch. But I also think it would make sense to be generic from the get go and make sure the architecture can support all use cases---because we already have so much learning from the existing controls.
It would be nice if we can focus on collecting all feature requests first and do an architecture review (maybe in a SIP?) before diving deep into implementing the full feature for a specific control type. This POC is a good starting point to kick off the conversation, but let's make sure we get things right to avoid wasted work.
I'm also not sure how to run user testing in this case, do we ask users to choose between DnD and click? Do we do that after all features for DnD are implemented or before? What if users pick click because DnD is not easy to use enough? These controls need an overhaul anyway so I can't imagine a case where we will abandon the new solution just because of some unfavorable use feedbacks (which are probably fixable).
It is also not true that we will need to spend 2x of time to maintain two sets of features. Current select controls can have a code freeze while we work on the new controls with more advanced features and better architecture. We would probably need that to stay focused.
I have added in PR description. |
options: { string: ColumnMeta }; | ||
} | ||
|
||
export default function DndColumnSelectLabel(props: LabelProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could make this component more generic, so we could reuse it with filters and metrics. It'd be great if we were able to pass optionsRenderer, children and other values as props. Though I don't know if it's in scope of this PR - next iteration maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good question!
Code reuse can refine the logic, but it may make the logic difficult to understand. We should iterate it in the future. We have to consider all kinds of components.
e.g.
- various Filter(datetime filter, numeric filter, text filter)
- various dimension selection(simple dimension, dimension set, dimension hierarchy)
The current DND should be just a prototype.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(regarding making a more generic component vs a specific component) Agree with both - this should be made generic, but as a first POC behind a FF I'm ok with this being more specific and making it more generic later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zhaoyongjie could you elaborate on what is a dimension set and dimension hierarchy? They sound like pretty advanced feature and I'm not sure they should be a concern in implementing the first draft of a generic component.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, but the code looks really good and I feel it works very well for a prototype 👍 I feel we need to iterate on the design/UX of "ghost button" vs "plus button" vs "dnd only" functionality when adding new columns/filters/metrics, as it might cause considerable work down the road if we change this later. Also, I know this is behind a FF now, but I still can't help but feel confused by some controls having the plus/ghost button but some only supporting dnd.
|
||
function placeHolderRenderer() { | ||
return ( | ||
<AddControlLabel cancelHover> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(regarding dnd only vs also supporting traditional clicking the control) I personally feel it's better to offer the users the option of using the traditional approach, especially as long there is a sensible control which you can click. While e.g. Tableau AFAIK only supports dnd interaction, in that UI it makes more sense, but in Superset I feel it's still a perfectly valid path to adding filters/metrics/columns.
return `dashed 1px ${theme.colors.info.dark1}`; | ||
} | ||
if (isOver && !canDrop) { | ||
return `dashed 1px ${theme.colors.error.dark1}`; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels like dashed 1px
doesn't stick out quite enough. IMO 2px
looked better, but then the default 1 px
causes this to resize when hovering in/out, so that doesn't work quite so well without addditional tuning of the default borders.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should also add an indicator for canDrop && !isOver
so that user knows where he can drag a column
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we should also add an indicator for
canDrop && !isOver
so that user knows where he can drag a column
Yeah I like that idea 👍
options: { string: ColumnMeta }; | ||
} | ||
|
||
export default function DndColumnSelectLabel(props: LabelProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(regarding making a more generic component vs a specific component) Agree with both - this should be made generic, but as a first POC behind a FF I'm ok with this being more specific and making it more generic later.
if (Array.isArray(values)) { | ||
groupByValues = values; | ||
this.isScalar = false; | ||
} else { | ||
groupByValues = values ? [values] : []; | ||
this.isScalar = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might just be me, but I feel the term scalar
can cause confusion here (in mathematics a scalar is a real number, which in the context of JS would be number
). I'd rather use isArray
to highlight that this refers to distinguishing between arrays and non-arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 on change this to isArray
check since we may want to support other more complex option
in the future (e.g. AdhocFilter
and AdhocMetric
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
scalar
in linear algebra, relative to vector
. refer to SQLAlachemy and Pandas variable naming.
isArray
more straightforward, I changed this variable name.
Additional to #13210 (comment) on whether we should keep
@villebro @kgabryje @ktmud @mistercrunch let's keep the discussion going so Design can have enough information to go on. but let's merge this PR soon as it's currently blocking follow up PRs. |
@junlincc This PR was ready for review for only 2 days. I think we should not rush to merge into If it's for convenience of reviewing and unblocking future PRs, let's make a feature branch in |
...rset-frontend/src/explore/components/controls/DndColumnSelectControl/utils/optionSelector.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some suggestions on how to make things more generic in the followups. Agreed this is a nice UX for a POC.
type: | ||
| typeof DatasourcePanelDndType.METRIC | ||
| typeof DatasourcePanelDndType.COLUMN; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should also work:
type: | |
| typeof DatasourcePanelDndType.METRIC | |
| typeof DatasourcePanelDndType.COLUMN; | |
type: DatasourcePanelDndType; |
...set-frontend/src/explore/components/controls/DndColumnSelectControl/DndColumnSelectLabel.tsx
Outdated
Show resolved
Hide resolved
}, | ||
|
||
canDrop: (item: DatasourcePanelDndItem) => | ||
!optionSelector.has(item.metricOrColumnName), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Example error messages:
- Column already added
- Cannot group by a metric
withCaret?: boolean; | ||
} | ||
|
||
export const GroupByItemType = 'groupByItem'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One way to make this more generic:
export const GroupByItemType = 'groupByItem'; | |
/** | |
* 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
orDndItemType.metricOption
is dropped to a group by (column select) control, reject and show a warning; - if a
DndItemType.column
orcolumnOption
is dropped to metric control, create an adhoc metric using this column. - If a
metric
,metricOption
,column
, orcolumnOption
is dragged to the adhoc filter control or sort by control, create a new filter using the metric or column.
*/ | ||
import { ColumnMeta } from '@superset-ui/chart-controls'; | ||
|
||
export class OptionSelector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's possible to make this generic, too, so you can support complex options (adhoc metrics, etc) in the list:
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.
|
||
interface LabelProps extends BaseControlConfig { | ||
name: string; | ||
value: string[] | string | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you may still need a isMulti
flag because null
could represents empty select for multiple selects as well.
🟢 |
Todo: for group-by that only accepts dimensions , add red indicator for metrics drop. @zhaoyongjie |
copy that, this feature need more polish, thank you. |
@yardz To reproduce
Screen.Recording.2021-03-01.at.8.41.30.AM.movthanks! |
* initial dnd * shift group by items * lint * fix shift options * wip * wip * fix shift action * support scalar dimentions * control rename to DndColumnSelectControl * remove unused files * added feature flag * ff to False by default * fix ut * lint * improve code smell * added indicator * replace value when column is scalcar * scalar to isArray * var rename * minor fix * update dependence * minor fix
SUMMARY
POC of Explore drag and drop query panel. scope - Groupby control only.
Todo:
This feature will be behind a FF until all drag & drop is implemented in all control fields.
added a new feature flage
The new FF name is
ENABLE_EXPLORE_DRAG_AND_DROP
, and the default value is False.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
multi columns groupby
multi-dimensions.mp4
single column groupby
scalar-dimension.mp4
TEST PLAN
ADDITIONAL INFORMATION