Skip to content

Conversation

@datability-io
Copy link
Contributor

@datability-io datability-io commented Feb 10, 2019

Currently, in the Select a visualization type dialog, visualization types are not sorted in any specific way. This PR sorts visualization types based on the follow rule:

If the user has created any slice in the past, those visualization types will be displayed first and are sorted based on that user's usage statistics.

For those visualization types that the user has never used before, they will be sorted based on the usage statistics across all users.

For those visualization types that have never been used by any user, they will show up in whatever original order that is currently using.

For a more constant user experience, the sort order is cached for 7 days.

@kristw kristw added the requires:more-info Requires more information from author label Feb 10, 2019
@datability-io datability-io changed the title initial commit for VIZ-58 Sort Chart Types based on Usage Feb 11, 2019
@mistercrunch
Copy link
Member

I think there's a good idea here, but dynamic ordering can be confusing to users. As you'd author your first few dashboards, ordering may move around before a real usage pattern emerges. "Why is the ordering changing every time I open this dialog?".

Also looking at the current logic, from my understanding I think the component ordering may shift under the user's mouse. Once the async request finishes (say if it takes 1-200ms) the user may see flickering.

My vote would go towards re-thinking the ordering based on overall usage, but probably leaving it static.

A few related ideas in this area:

  • categorize the visualizations in the modal
  • add a "quick toggle" for popular types near the label so that users don't have to open the modal to switch across common types

@datability-io
Copy link
Contributor Author

The order is actually cached for 7 days, so it won't change every time the user opens up the dialog, but I'm ok switching to overall usage. I was more considering those users who might be more likely to use visualization types that are less popular. I was also thinking to have two sections. One for popular ones used by the current users and the other by the overall usage stats.

I will re-think about the async call issue.

@betodealmeida
Copy link
Member

@mistercrunch what if we show a loading indicator while the AJAX call is in flight? I assume that for most people the AJAX call will finish before they open the visualizations menu.

Also, IMHO the benefits of sorting by popularity outweigh the confusion brought by changing the order. @dorq, can you give us some insight here?

One alternative we discussed is categorizing the vizs in 2 blocks:

  • My visualizations (sorted by user usage)
  • Company visualizations (sorted by company usage)

Dividing it this way I think there's a bigger expectancy that the order will change, since as you creat charts with new vizs they will populate the first section.

kristw
kristw previously requested changes Feb 13, 2019
@kristw kristw added enhancement:request Enhancement request submitted by anyone from the community and removed requires:more-info Requires more information from author labels Feb 13, 2019
@dorq
Copy link

dorq commented Feb 18, 2019

@betodealmeida Thanks for tagging me in this. Can I see a screenshot (or try this live somewhere) where I can see the two sections that both you and @datability-io alluded to?

@mistercrunch
Copy link
Member

My take on this one is that it's good to have things generally sorted / organized based on usage (or by categories), but that it's not worth doing something dynamic. We could do a one-off analysis and hard-code and it'd be just fine. I'd say the extra mechanics & async call wait are not worth it.

If really we want a better visualization picker there are other things we can do and I'd look at designers for input.

@datability-io
Copy link
Contributor Author

I will make it static for now then.

@codecov-io
Copy link

codecov-io commented Feb 27, 2019

Codecov Report

Merging #6849 into master will increase coverage by 8.47%.
The diff coverage is 90.47%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6849      +/-   ##
==========================================
+ Coverage   56.32%   64.79%   +8.47%     
==========================================
  Files         527      421     -106     
  Lines       23528    20404    -3124     
  Branches     2781     2251     -530     
==========================================
- Hits        13251    13220      -31     
+ Misses       9866     7051    -2815     
+ Partials      411      133     -278
Impacted Files Coverage Δ
...src/explore/components/controls/VizTypeControl.jsx 81.81% <90.47%> (+3.55%) ⬆️
superset/assets/src/query/index.ts 0% <0%> (-100%) ⬇️
superset/assets/src/query/buildQueryObject.ts 0% <0%> (-100%) ⬇️
superset/assets/src/query/buildQueryContext.ts 0% <0%> (-100%) ⬇️
superset/assets/src/query/Metric.ts 0% <0%> (-100%) ⬇️
superset/assets/src/query/FormData.ts 0% <0%> (-100%) ⬇️
superset/assets/src/query/Column.ts 0% <0%> (-100%) ⬇️
superset/assets/src/query/DatasourceKey.ts 0% <0%> (-92.31%) ⬇️
...rset/assets/src/explore/reducers/exploreReducer.js 35.71% <0%> (-12.57%) ⬇️
...erset/assets/src/dashboard/reducers/datasources.js 57.14% <0%> (-9.53%) ⬇️
... and 235 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 0b1fbf8...995d50e. Read the comment docs.

@betodealmeida betodealmeida dismissed kristw’s stale review February 27, 2019 21:30

All issues addressed

Copy link
Member

@betodealmeida betodealmeida left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just a few comments on making the for loops more JS friendly.

@betodealmeida betodealmeida merged commit 8346e62 into apache:master Mar 5, 2019
@kristw
Copy link
Contributor

kristw commented Mar 5, 2019

I am late to this but could I suggest extracting the default order into a separate file?
This will let each organization overrides the default order of the vis.

@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 First shipped in 0.34.0 labels Feb 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels enhancement:request Enhancement request submitted by anyone from the community 🚢 0.34.0 First shipped in 0.34.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants