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

feat(plugin-chart-echarts): [feature-parity] support Extra Control on the time-series area chart #1336

Closed

Conversation

stephenLYZ
Copy link
Contributor

@stephenLYZ stephenLYZ commented Aug 29, 2021

🏆 Enhancements

This PR implements Extra Control on the time-series area chart, and the chart's update mechanism is using setControlValue hook. In addition, I changed the stack checkbox control to select control. When Extra Control is updated, the control panel will be updated as well, and vice versa. (this is different from NVD3, where updating extra control does not update the control panel). Since 'Stacked' and 'Expended' seem to be used more often, I only keep these two options.
Another UI difference is that the button is used instead of the radio to maintain consistency with the control panel.

Refers to :

cc @villebro @zhaoyongjie @junlincc

explore

before

2021-08-29.8.49.16.mov

after

2021-08-29.8.51.51.mov

dashboard

before

2021-08-29.8.53.38.mov

after

2021-08-29.8.54.37.mov

@stephenLYZ stephenLYZ requested a review from a team as a code owner August 29, 2021 11:53
@vercel
Copy link

vercel bot commented Aug 29, 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/2CSgVuvAhrT6gaJNY7TpdHvSRsSC
✅ Preview: https://superset-ui-git-fork-stephenlyz-feat-area-extra-edc0da-superset.vercel.app

[Deployment for 1247c74 failed]

@stephenLYZ stephenLYZ changed the title feat(plugin-chart-echarts): [feature-parity] support Extra Control on the area chart feat(plugin-chart-echarts): [feature-parity] support Extra Control on the time-series area chart Aug 29, 2021
@codecov
Copy link

codecov bot commented Aug 29, 2021

Codecov Report

Merging #1336 (5243a8e) into master (852fb2c) will decrease coverage by 0.44%.
The diff coverage is 40.42%.

❗ Current head 5243a8e differs from pull request most recent head 1247c74. Consider uploading reports for the commit 1247c74 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1336      +/-   ##
==========================================
- Coverage   30.43%   29.98%   -0.45%     
==========================================
  Files         497      489       -8     
  Lines       10000     9882     -118     
  Branches     1689     1666      -23     
==========================================
- Hits         3043     2963      -80     
+ Misses       6711     6676      -35     
+ Partials      246      243       -3     
Impacted Files Coverage Δ
.../shared-controls/components/RadioButtonControl.tsx 0.00% <0.00%> (ø)
...hart-echarts/src/MixedTimeseries/transformProps.ts 0.00% <ø> (ø)
.../plugin-chart-echarts/src/MixedTimeseries/types.ts 100.00% <ø> (ø)
...chart-echarts/src/Timeseries/Area/controlPanel.tsx 50.00% <ø> (ø)
...chart-echarts/src/Timeseries/EchartsTimeseries.tsx 0.00% <0.00%> (ø)
...ugins/plugin-chart-echarts/src/Timeseries/types.ts 100.00% <ø> (ø)
plugins/plugin-chart-echarts/src/types.ts 100.00% <ø> (ø)
...gin-chart-echarts/src/Timeseries/transformProps.ts 50.00% <50.00%> (-5.13%) ⬇️
plugins/plugin-chart-echarts/src/utils/series.ts 89.87% <78.57%> (-2.89%) ⬇️
plugins/plugin-chart-echarts/src/constants.ts 100.00% <100.00%> (ø)
... and 61 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 722cd5a...1247c74. Read the comment docs.

@junlincc
Copy link
Contributor

@stephenLYZ thank you for the improvement! we probably dont need the buttons at the top - the original one is pretty much a design debt. and i wonder how it goes with legend enabled?

@villebro wdyt?

@jhult
Copy link

jhult commented Oct 11, 2021

@stephenLYZ and @villebro, any news on this? We would love to see this get merged.

Would this also cover bar charts? If not, once this is merged, could that be easily added?

@stephenLYZ
Copy link
Contributor Author

@jhult yeah. In the next step I will style it to be compatible with the previous design, and then open another PR to cover the bar chart

@stephenLYZ
Copy link
Contributor Author

Since we did the migration of the superset mono repo, all the changes are put here apache/superset#16493.

@villebro
Copy link
Contributor

The codebase on this repo has been moved to the main Apache Superset repo, and consequently the repo is in the process of being archived. See the Superset Improvement Proposal for details: apache/superset#13013 . While all currently open issues and PRs will be closed, we encourage you to reopen this PR on the main repo, which should be as simple as moving over any code changes as follows:

  • core packages: packages/** (this repo) -> superset-frontend/packages/** (main repo)
  • plugins: plugins/** (this repo) -> superset-frontend/plugins/** (main repo)

If you need help with the migration, please post a message on the SIP or reach out on the community Slack.

@villebro villebro closed this Dec 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants