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

feat(legacy-plugin-chart-nvd3): add control panels #469

Merged
merged 2 commits into from
May 12, 2020

Conversation

suddjian
Copy link
Member

@suddjian suddjian commented May 8, 2020

All the ones that are actually used in superset, anyway

@suddjian suddjian requested a review from a team as a code owner May 8, 2020 07:41
@vercel
Copy link

vercel bot commented May 8, 2020

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/e0yrs5fh6
✅ Preview: https://superset-ui-git-fork-suddjian-nvd3-controls.superset.now.sh

@kristw kristw changed the title feat(plugin-chart-nvd3): add control panels feat(legacy-plugin-chart-nvd3): add control panels May 8, 2020
timeSeriesSection[1],
sections.annotations,
],
controlOverrides: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we apply these overrides in-place instead of having this section here?

['color_scheme', 'label_colors']
[
  'color_scheme', // replace with the object and set renderTrigger to false
  'label_colors'
]

Copy link
Member Author

Choose a reason for hiding this comment

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

I think that's a good idea, but for now I'm just trying to move the control panels while keeping them as intact as possible to minimize the chance of breaking things.

Copy link
Member

Choose a reason for hiding this comment

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

@kristw I'm making an internal JIRA ticket to track this, and we can go through and audit and remove (where possible) all controlOverrides. If you think it's safe enough, we can then consider removing the code that enables them, as well. We'll discuss further with you :)

@codecov
Copy link

codecov bot commented May 11, 2020

Codecov Report

Merging #469 into master will decrease coverage by 0.00%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #469      +/-   ##
=========================================
- Coverage    3.19%   3.19%   -0.01%     
=========================================
  Files         152     154       +2     
  Lines        5252    5259       +7     
  Branches      280     282       +2     
=========================================
  Hits          168     168              
- Misses       5047    5054       +7     
  Partials       37      37              
Impacted Files Coverage Δ
plugins/legacy-preset-chart-nvd3/src/Area/index.js 0.00% <ø> (ø)
plugins/legacy-preset-chart-nvd3/src/Bar/index.js 0.00% <ø> (ø)
...ugins/legacy-preset-chart-nvd3/src/Bubble/index.js 0.00% <ø> (ø)
...ugins/legacy-preset-chart-nvd3/src/Bullet/index.js 0.00% <ø> (ø)
...gins/legacy-preset-chart-nvd3/src/Compare/index.js 0.00% <ø> (ø)
...gins/legacy-preset-chart-nvd3/src/DistBar/index.js 0.00% <ø> (ø)
...ins/legacy-preset-chart-nvd3/src/DualLine/index.js 0.00% <ø> (ø)
plugins/legacy-preset-chart-nvd3/src/Line/index.js 0.00% <ø> (ø)
...cy-preset-chart-nvd3/src/LineMulti/controlPanel.ts 0.00% <0.00%> (ø)
...ns/legacy-preset-chart-nvd3/src/LineMulti/index.js 0.00% <ø> (ø)
... and 7 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 69a004c...92667cf. Read the comment docs.

Copy link
Member

@rusackas rusackas left a comment

Choose a reason for hiding this comment

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

LGTM... we can keep dialing things in on this side (to address Krist's thoughts) once it's all moved over.

@rusackas rusackas merged commit ad267db into apache-superset:master May 12, 2020
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