feat: Implement drag and drop for metrics #13575

kgabryje · 2021-03-11T13:42:56Z

SUMMARY

This PR implements creating adhoc metrics by dragging columns and saved metrics from datasource panel to metrics control.
When user drops a column or saved metric, a popover opens pre-filled with values - if a metric was dropped, a "Saved metrics" tab is open by default with a proper metric selected, otherwise "Simple" tab is open with a column selected.
The old functionality of creating a metric by clicking a "plus" button has ben removed.
To use the feature, enable ENABLE_EXPLORE_DRAG_AND_DROP feature flag and use changes from apache-superset/superset-ui#1004.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Screen.Recording.2021-03-11.at.13.57.14.mov

TEST PLAN

ADDITIONAL INFORMATION

CC: @villebro @junlincc @zhaoyongjie

codecov · 2021-03-11T14:33:18Z

Codecov Report

Merging #13575 (561951a) into master (5fca19d) will decrease coverage by 4.34%.
The diff coverage is 14.07%.

@@            Coverage Diff             @@
##           master   #13575      +/-   ##
==========================================
- Coverage   77.53%   73.19%   -4.35%     
==========================================
  Files         904      604     -300     
  Lines       45986    21529   -24457     
  Branches     5552     5682     +130     
==========================================
- Hits        35657    15758   -19899     
+ Misses      10195     5636    -4559     
- Partials      134      135       +1

Flag	Coverage Δ
cypress	`56.84% <7.43%> (-0.37%)`	⬇️
hive	`?`
javascript	`63.08% <14.17%> (-0.31%)`	⬇️
mysql	`?`
postgres	`?`
presto	`?`
python	`?`
sqlite	`?`

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

Impacted Files	Coverage Δ
...controls/DndColumnSelectControl/DndSelectLabel.tsx	`36.84% <0.00%> (-2.05%)`	⬇️
...s/controls/MetricControl/MetricDefinitionValue.jsx	`100.00% <ø> (+5.88%)`	⬆️
...-frontend/src/explore/components/controls/index.js	`100.00% <ø> (ø)`
...ontrols/DndColumnSelectControl/DndMetricSelect.tsx	`10.40% <10.40%> (ø)`
...ontrols/DndColumnSelectControl/DndFilterSelect.tsx	`9.23% <50.00%> (ø)`
...ntrols/MetricControl/AdhocMetricPopoverTrigger.tsx	`93.33% <75.00%> (-1.41%)`	⬇️
...omponents/controls/DndColumnSelectControl/index.ts	`100.00% <100.00%> (ø)`
superset/db_engine_specs/solr.py
superset/connectors/connector_registry.py
superset/common/query_context.py
... and 299 more

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 5fca19d...54be16b. Read the comment docs.

junlincc

This is great! thank you so much for the work! 🙏
just one comment, do we still need to open the popover + clicking save when a saved metrics is dropped?

@mihir174 @srinify thoughts?

ktmud · 2021-03-11T20:47:06Z

I think we do still need that ghost button to indicate users can drop more metrics/columns.

junlincc · 2021-03-11T20:51:43Z

@kgabryje for your context - the only concern about the ghost button is the additional vertical space they take in each drop field.
#12631,
#13071

Steejay · 2021-03-11T21:03:04Z

re the + button: I think having a secondary way to add metrics besides dragging and dropping is still important especially when considering accessibility. Having a simple click button/action provides users that may have some physical disability (whether temporary, situational or permanent) a way to complete the action more easily. Can the ghost button persist even when the field is populated with a metric(s)?

junlincc · 2021-03-11T22:08:24Z

Hi @Steejay thanks for chiming in.

I'm afraid that click-to-select will not support some of the new advanced features we are adding e.g. hierarchy at all. from business standpoint, we are also not gonna invest in maintaining two sets of feature that basically do the same thing. there are more reasons why we need to completely move away from select, comments can be found in
#13210
I understand accessibility e.g. color blind, is the main concern, and we need your help to solve it within DND.

Can the ghost button persist even when the field is populated with a metric(s)?
I believe that's what mud suggested. I'm not sure that's ideal though.

ktmud · 2021-03-12T01:47:04Z

If users can click the persistent ghost button to add an item, then the "+" button on the right corner would not be needed.

zhaoyongjie · 2021-03-12T02:22:59Z

We should replace all the metric-like target control, such as sort by, percentage metric. but this is enough for a POC.

zhaoyongjie · 2021-03-12T02:28:11Z

Some corner issue in title

dnd-metric-issue.mp4

zhaoyongjie

LGTM, for the POC.

junlincc · 2021-03-12T02:30:22Z

Some corner issue in title

no need to worry about it, those issues will all be addressed in refactoring and redesign.

junlincc · 2021-03-12T02:43:19Z

If users can click the persistent ghost button to add an item, then the "+" button on the right corner would not be needed.

Reason why we may need the blue button -> accessibility
Reason why we may need the ghost button -> indicate droppable field

It is not about where or what to click. The goal is to solve these two above within dnd, and get rid of click-to-select!!!

zhaoyongjie · 2021-03-12T04:33:56Z

@kgabryje please bumping the latest superset-ui to the main repo.

ktmud · 2021-03-12T05:30:06Z

@junlincc I don't understand why the goal is get rid of click-to-select, though. As I responded in the earlier PR, there seems to be a counter-argument to everything you mentioned.

Hierarchical columns sounds like a very advanced feature that I'm not even sure what it is or why we need it, but I'd imagine we could get something working in a popover eventually. Have we heard from designers that it is not possible to add these features without drag & drop?

Re: accessibility, it's more than just colorblind and visual-impairment, but also easiness to interact. As @Steejay mentioned, DnD simply isn't possible for some users in certain scenarios (e.g. people with poor motor control). An DnD only UI will never achieve the same level of accessibility than a clickable Ui.

junlincc · 2021-03-12T06:48:48Z

@ktmud all your counter arguments make sense.
Questions:
Have we done any research on the real need of addressing accessibility issue? Is the number big enough to justify investing 2X(engineer + design + product)to achieve the same feature goal? Superset is an OSS so to me, even 0.1% of users is a big enough number. Those challenges need to be categorized and looked at separately. And have we done enough exploration to prove that there's absolutely no way to solve these challenges within DND? What about keyboard shortcut? Have we done research on how the biggest BI tool in the market,Tableau, solved these challenges? @Steejay @mihir174

we have been constantly balancing requests for fixing broken NVD3 charts vs adding new Echarts, fixing broken FilterBox vs enhancing new native filter. We all have the same goal which is to provide the best UX to our users(i take back my statement of 'our goal is to get rid of select') It's the divided attention or lack of focus stopping us from getting to the ultimate goal.

villebro

I had a pending review from last night - leaving the comments here as ideas for future PRs.

villebro · 2021-03-12T00:01:53Z

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

+      .map(metric =>
+        (metric as AdhocMetric).column
+          ? (metric as AdhocMetric).column.column_name
+          : (metric as ColumnMeta).column_name || metric,
+      )
+      .filter(name => name && typeof name === 'string')
+      .every(name => columnNames.has(name))


It would be nice to add type guards for the types in value to be able to separate between the different metrics types (string | AdhocMetric | ColumnMeta). We could then add a getMetricName(metric) helper function which would return the name based on which type it is.

villebro · 2021-03-12T00:09:02Z

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

+import { logging, SupersetClient, t } from '@superset-ui/core';
 import { ColumnMeta, Metric } from '@superset-ui/chart-controls';


Bycatch: I believe Metric is originally defined in @superset-ui/core (probably better to import from there, as I'm unsure why it's exported by @superset-ui/chart-controls)

kgabryje · 2021-03-12T10:23:51Z

@junlincc @ktmud @mihir174 I like the idea of displaying the ghost button to indicate that the user can drop more columns and metrics. However, I feel that it requires a design input as in my opinion it looks a bit strange when we display values and the ghost button in current form. Details like misaligned "+" and "x" buttons, maybe a dashed border might need some polishing to make the component look more appealing. WDYT?

kgabryje · 2021-03-12T10:30:52Z

However, if you think that the ghost button looks fine as it is, I'll open a PR with that change

villebro · 2021-03-12T10:43:51Z

FWIW, I think the ghost button is awesome, much better than the plus button, both aesthetically and functionally (but I agree it needs some polish).

kgabryje · 2021-03-12T10:45:59Z

Another potential improvement is to display coloured dash border when user starts dragging an element, not only when it's hovering over a control component. That might help users understand where they can drop columns and metrics

Screen.Recording.2021-03-12.at.11.43.40.mov

kgabryje · 2021-03-12T10:48:16Z

just one comment, do we still need to open the popover + clicking save when a saved metrics is dropped?

@junlincc Good idea! I'll try to include that in a follow-up PR

zhaoyongjie · 2021-03-12T15:01:45Z

Another potential improvement is to display coloured dash border when user starts dragging an element, not only when it's hovering over a control component. That might help users understand where they can drop columns and metrics
Screen.Recording.2021-03-12.at.11.43.40.mov

I like this design.

villebro · 2021-03-12T15:06:52Z

Another potential improvement is to display coloured dash border when user starts dragging an element, not only when it's hovering over a control component. That might help users understand where they can drop columns and metrics
Screen.Recording.2021-03-12.at.11.43.40.mov

I like this design.

Yeah, me too. I was just thinking to myself that when fully and properly implemented (good scrolling/filtering on column/metric tab, highlighting during dragging etc), DND offers so many small UX improvements that it make the select feature seem almost confusing and awkward. Not sure what I said a few weeks ago, but after careful deliberation, I vote for deprecating the select control as soon as possible :)

ktmud · 2021-03-13T00:31:58Z

@ktmud all your counter arguments make sense.
Questions:
Have we done any research on the real need of addressing accessibility issue? Is the number big enough to justify investing 2X(engineer + design + product)to achieve the same feature goal? Superset is an OSS so to me, even 0.1% of users is a big enough number. Those challenges need to be categorized and looked at separately. And have we done enough exploration to prove that there's absolutely no way to solve these challenges within DND? What about keyboard shortcut? Have we done research on how the biggest BI tool in the market,Tableau, solved these challenges? @Steejay @mihir174

we have been constantly balancing requests for fixing broken NVD3 charts vs adding new Echarts, fixing broken FilterBox vs enhancing new native filter. We all have the same goal which is to provide the best UX to our users(i take back my statement of 'our goal is to get rid of select') It's the divided attention or lack of focus stopping us from getting to the ultimate goal.

There is a movement called Inclusive Design I'm not sure if you have heard of. Basically it says instead of thinking things like accessibility as an investment that benefits only a small fraction of users, we should have the mindset of being inclusive from the very beginning. It doesn't necessary slow you down in the process, but will help you reach to more users.

I have yet to see how supporting click-to-select would slow things down or introduce the so-called 2x maintenance cost. If we want to get rid of it because it doesn't sit well with the DnD experience, that's fine, but in terms of code maintenance, I find it hard to believe it'd be 2x the effort.

* Convert MetricsControl to functional component * Implement drag and drop for metrics * Implement customizable placeholder for DndSelectLabel

kgabryje added 3 commits March 11, 2021 10:55

Convert MetricsControl to functional component

c02d1e8

Implement drag and drop for metrics

459dba2

Implement customizable placeholder for DndSelectLabel

54be16b

pull-request-size bot added the size/L label Mar 11, 2021

junlincc requested review from villebro, junlincc and zhaoyongjie March 11, 2021 20:44

junlincc reviewed Mar 11, 2021

View reviewed changes

junlincc added the need:design-review Requires input/approval from a Designer label Mar 11, 2021

junlincc added the explore:drag&drop Related to drag&drop in Explore label Mar 11, 2021

zhaoyongjie approved these changes Mar 12, 2021

View reviewed changes

zhaoyongjie merged commit d439da2 into apache:master Mar 12, 2021

villebro reviewed Mar 12, 2021

View reviewed changes

zhaoyongjie mentioned this pull request Mar 12, 2021

chore(explore): bump superset-ui 0.17.19 #13593

Merged

6 tasks

kgabryje mentioned this pull request Mar 12, 2021

feat(explore): Drag and drop UX improvements #13598

Merged

6 tasks

ktmud mentioned this pull request Mar 17, 2021

fix: clean up incorrect usage of TypeScript type in PropTypes #13681

Merged

6 tasks

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 for metrics #13575

feat: Implement drag and drop for metrics #13575

kgabryje commented Mar 11, 2021

codecov bot commented Mar 11, 2021

junlincc left a comment

ktmud commented Mar 11, 2021 •

edited

Loading

junlincc commented Mar 11, 2021

Steejay commented Mar 11, 2021

junlincc commented Mar 11, 2021 •

edited

Loading

ktmud commented Mar 12, 2021

zhaoyongjie commented Mar 12, 2021

zhaoyongjie commented Mar 12, 2021

zhaoyongjie left a comment

junlincc commented Mar 12, 2021

junlincc commented Mar 12, 2021 •

edited

Loading

zhaoyongjie commented Mar 12, 2021 •

edited

Loading

ktmud commented Mar 12, 2021

junlincc commented Mar 12, 2021 •

edited

Loading

villebro left a comment

villebro Mar 12, 2021

villebro Mar 12, 2021

kgabryje commented Mar 12, 2021

kgabryje commented Mar 12, 2021

villebro commented Mar 12, 2021

kgabryje commented Mar 12, 2021

kgabryje commented Mar 12, 2021

zhaoyongjie commented Mar 12, 2021

villebro commented Mar 12, 2021

ktmud commented Mar 13, 2021

		import { logging, SupersetClient, t } from '@superset-ui/core';
		import { ColumnMeta, Metric } from '@superset-ui/chart-controls';

feat: Implement drag and drop for metrics #13575

feat: Implement drag and drop for metrics #13575

Conversation

kgabryje commented Mar 11, 2021

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

codecov bot commented Mar 11, 2021

Codecov Report

junlincc left a comment

Choose a reason for hiding this comment

ktmud commented Mar 11, 2021 • edited Loading

junlincc commented Mar 11, 2021

Steejay commented Mar 11, 2021

junlincc commented Mar 11, 2021 • edited Loading

ktmud commented Mar 12, 2021

zhaoyongjie commented Mar 12, 2021

zhaoyongjie commented Mar 12, 2021

zhaoyongjie left a comment

Choose a reason for hiding this comment

junlincc commented Mar 12, 2021

junlincc commented Mar 12, 2021 • edited Loading

zhaoyongjie commented Mar 12, 2021 • edited Loading

ktmud commented Mar 12, 2021

junlincc commented Mar 12, 2021 • edited Loading

villebro left a comment

Choose a reason for hiding this comment

villebro Mar 12, 2021

Choose a reason for hiding this comment

villebro Mar 12, 2021

Choose a reason for hiding this comment

kgabryje commented Mar 12, 2021

kgabryje commented Mar 12, 2021

villebro commented Mar 12, 2021

kgabryje commented Mar 12, 2021

kgabryje commented Mar 12, 2021

zhaoyongjie commented Mar 12, 2021

villebro commented Mar 12, 2021

ktmud commented Mar 13, 2021

ktmud commented Mar 11, 2021 •

edited

Loading

junlincc commented Mar 11, 2021 •

edited

Loading

junlincc commented Mar 12, 2021 •

edited

Loading

zhaoyongjie commented Mar 12, 2021 •

edited

Loading

junlincc commented Mar 12, 2021 •

edited

Loading