Skip to content
This repository has been archived by the owner on Dec 10, 2021. It is now read-only.

feat(plugin-chart-echarts): echarts funnel chart #1006

Merged
merged 16 commits into from
May 2, 2021
Merged

feat(plugin-chart-echarts): echarts funnel chart #1006

merged 16 commits into from
May 2, 2021

Conversation

xiezhongfu
Copy link
Contributor

@xiezhongfu xiezhongfu commented Mar 13, 2021

add funnel of echarts and the demo into plugin-chart-echarts

this is a link from funnel of echarts.apache: https://echarts.apache.org/examples/en/editor.html?c=funnel
this is a gif of funnel in superset

Untitled

@xiezhongfu xiezhongfu requested a review from a team as a code owner March 13, 2021 13:33
@vercel
Copy link

vercel bot commented Mar 13, 2021

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/superset/superset-ui/ERWAfh3uY9n7unxmqiefvzRpMqkH
✅ Preview: https://superset-ui-git-fork-xiezhongfu-master-superset.vercel.app

@codecov
Copy link

codecov bot commented Mar 13, 2021

Codecov Report

Merging #1006 (3b394c2) into master (6825e87) will increase coverage by 0.16%.
The diff coverage is 50.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1006      +/-   ##
==========================================
+ Coverage   27.80%   27.96%   +0.16%     
==========================================
  Files         453      459       +6     
  Lines        9104     9169      +65     
  Branches     1416     1429      +13     
==========================================
+ Hits         2531     2564      +33     
- Misses       6381     6409      +28     
- Partials      192      196       +4     
Impacted Files Coverage Δ
.../plugin-chart-echarts/src/Funnel/EchartsFunnel.tsx 0.00% <0.00%> (ø)
plugins/plugin-chart-echarts/src/Funnel/index.ts 0.00% <0.00%> (ø)
...s/plugin-chart-echarts/src/Funnel/controlPanel.tsx 66.66% <66.66%> (ø)
.../plugin-chart-echarts/src/Funnel/transformProps.ts 74.35% <74.35%> (ø)
...gins/plugin-chart-echarts/src/Funnel/buildQuery.ts 100.00% <100.00%> (ø)
plugins/plugin-chart-echarts/src/Funnel/types.ts 100.00% <100.00%> (ø)
... and 2 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 6825e87...3b394c2. Read the comment docs.

Copy link
Contributor

@ktmud ktmud left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! This looks good. Can't wait to see it in real action.

plugins/plugin-chart-echarts/src/Funnel/types.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx Outdated Show resolved Hide resolved
@junlincc junlincc changed the title feat(plugin-chart-echarts): add funnel feat(plugin-chart-echarts): echarts funnel chart Mar 14, 2021
@junlincc
Copy link
Contributor

@xiezhongfu Thank you Zhongfu, for your contribution! Would you mind attaching a video? I can help you update the description accordingly

Copy link
Contributor

@villebro villebro left a comment

Choose a reason for hiding this comment

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

Some comments. In addition to the recommendations posted here, I believe we should consider removing the last cone in the funnel: for a sales funnel starting at 100 and ending in 20 closed cases, the funnel now looks like this:
image
Here it looks like the last step goes to zero, when in fact the funnel closes culminates in a 20 % closing rate. I think it might be more intuitive to show the last step to start and end in 20, making it essentially a box. Alternatively the funnel could be changed so that it only consists of two trapezoids, and the labels would be "cold call -> follow-up meeting" and "follow-up meeting -> sale" or similar (maybe this could be a customization option).

plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Funnel/buildQuery.ts Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx Outdated Show resolved Hide resolved
plugins/plugin-chart-echarts/src/Funnel/controlPanel.tsx Outdated Show resolved Hide resolved
Copy link
Member

@suddjian suddjian left a comment

Choose a reason for hiding this comment

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

The code all looks good! There are several changes suggested by myself and others in controlPanel.tsx that I think should be addressed. I'd be happy to help with those modifications, too!

Thank you for the awesome contribution.

@zhaoyongjie
Copy link
Contributor

image

Please resize thumbnail for Funnel Chart, Thanks.

@zhaoyongjie
Copy link
Contributor

sort and orient control can be aligned vertically

image

@xiezhongfu
Copy link
Contributor Author

The code all looks good! There are several changes suggested by myself and others in controlPanel.tsx that I think should be addressed. I'd be happy to help with those modifications, too!

Thank you for the awesome contribution.

Thank you. I try to solve the bug in controlPanel.tsx because I bootstrap superset-frontend in superset after I solved the DNS problem.
You can try those modifications.
SORRY for response the comment after a lone time.

@xiezhongfu xiezhongfu closed this Apr 24, 2021
yarn.lock Outdated Show resolved Hide resolved
Copy link
Contributor

@zhaoyongjie zhaoyongjie left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your first contribution!

@zhaoyongjie zhaoyongjie merged commit 2eb7c81 into apache-superset:master May 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants