Skip to content
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

[New-Viz][AmericanExpress] Funnel/Step Visualization #8358

Closed
wants to merge 1 commit into from

Conversation

TheLastSultan
Copy link
Contributor

@TheLastSultan TheLastSultan commented Oct 9, 2019

CATEGORY

  • Bug Fix
  • Enhancement (new features, refinement)
  • Refactor
  • Add tests
  • Build / Development Environment
  • Documentation

SUMMARY

This is a PR to our most popular viz_type at American Express. The Step/Funnel Visualization is a 2 in 1 visualization that is simple to use and highly modifiable. It has unique support of multi-pass queries and thus is worthy of the PR effort. The 'Funnel' feature can be turned on or off if the user prefers a simple step visualization over 'funnel'.

Funnel-Mode

image

Step-Mode

image

Control Panel Supports Multi-Step Queries with 'Add Step'

image

ADDITIONAL INFORMATION

  • 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

REVIEWERS

@TheLastSultan TheLastSultan force-pushed the amex_funnel_viz branch 3 times, most recently from f0005c7 to 91a495d Compare October 9, 2019 22:42
@villebro
Copy link
Member

Would you be able to post some screenshots? I think step charts would be an awesome addition, as they seem to be all the rage for decomposing financials these days.

@etr2460
Copy link
Member

etr2460 commented Oct 10, 2019

Linking this comment here: #8346 (comment)

Maybe this should be in a separate plugin instead so that we don't update the deprecated viz.py file

@codecov-io
Copy link

codecov-io commented Oct 11, 2019

Codecov Report

Merging #8358 into master will decrease coverage by 0.57%.
The diff coverage is 21.63%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8358      +/-   ##
==========================================
- Coverage   67.46%   66.88%   -0.58%     
==========================================
  Files         448      455       +7     
  Lines       22527    22869     +342     
  Branches     2364     2424      +60     
==========================================
+ Hits        15197    15296      +99     
- Misses       7192     7435     +243     
  Partials      138      138
Impacted Files Coverage Δ
...et/assets/src/explore/components/controls/index.js 100% <ø> (ø) ⬆️
...et/assets/src/visualizations/presets/MainPreset.js 0% <ø> (ø) ⬆️
...perset/assets/src/visualizations/Funnel/Funnel.jsx 0% <0%> (ø)
superset/assets/src/explore/controls.jsx 38.23% <0%> (-3.04%) ⬇️
...ssets/src/visualizations/Funnel/FunnelContainer.js 0% <0%> (ø)
...assets/src/visualizations/Funnel/transformProps.js 0% <0%> (ø)
superset/assets/src/setup/setupPlugins.ts 0% <0%> (ø) ⬆️
...ets/src/visualizations/Funnel/FunnelChartPlugin.js 0% <0%> (ø)
...rset/assets/src/explore/controlPanels/sections.jsx 100% <100%> (ø) ⬆️
.../assets/src/explore/components/controls/Filter.jsx 14.54% <14.54%> (ø)
... and 15 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 9a186ff...dce602c. Read the comment docs.

@TheLastSultan TheLastSultan changed the title [IN-Progress][AmEx] Funnel/Step Visualization [New-Viz][AmericanExpress] Funnel/Step Visualization Oct 11, 2019
@TheLastSultan
Copy link
Contributor Author

Updated @villebro . Just finishing some cleanup efforts and waiting to pass build.

@mistercrunch
Copy link
Member

We're in the process of moving all visualizations to live as plugins in other repos. This is very in-flight at the moment. Started writing an example here:
apache-superset/superset-ui-plugins#216

Happy to get on a call to talk about it and help you through the process. You can ping me on the Superset Slack.

@TheLastSultan
Copy link
Contributor Author

@mistercrunch amazing! Viz.py was getting a little out of hand to say the least :)

Although it is much needed and welcome, "in-flight" is apt for where superset-ui-plugin effort is at the moment. It would be much less time consuming to write a plugin once the legacy plugins are up and functional. Would it be possible to revisit that refactor when the time comes ?

@TheLastSultan
Copy link
Contributor Author

Bump @etr2460 @mistercrunch

@TheLastSultan
Copy link
Contributor Author

@etr2460 It's been 10+ days. What is the status of this PR? Can we get some reviewers on it please.

@mistercrunch
Copy link
Member

@TheLastSultan , @kristw did a lot of work to migrate all of the visualizations out of the repo, I'm afraid this is a requirement at this point. The closest thing to documentation on how to do this is apache-superset/superset-ui-plugins#216 at this time.

@stale
Copy link

stale bot commented Dec 31, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions. For admin, please label this issue .pinned to prevent stale bot from closing the issue.

@stale stale bot added the inactive Inactive for >= 30 days label Dec 31, 2019
@stale stale bot closed this Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
inactive Inactive for >= 30 days
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants