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

[reviewable] [refactor] Split visTypes into one file for each visualization type #6290

Merged
merged 7 commits into from
Nov 8, 2018

Conversation

kristw
Copy link
Contributor

@kristw kristw commented Nov 7, 2018

For maintainability, and future compatibility with plugins.

  • Rename visTypes.jsx to controlPanelConfigs.js
  • Split original content into one file for each visualization under controlPanels/ directory.

@williaster @conglei @graceguo-supercat @michellethomas @xtinec @mistercrunch

@kristw kristw changed the title [refactor] Split visTypes into one file for each visualization type [reviewable] [refactor] Split visTypes into one file for each visualization type Nov 7, 2018
@kristw kristw changed the title [reviewable] [refactor] Split visTypes into one file for each visualization type [refactor] Split visTypes into one file for each visualization type Nov 7, 2018
@codecov-io
Copy link

codecov-io commented Nov 7, 2018

Codecov Report

Merging #6290 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6290   +/-   ##
=======================================
  Coverage   77.33%   77.33%           
=======================================
  Files          67       67           
  Lines        9578     9578           
=======================================
  Hits         7407     7407           
  Misses       2171     2171

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 4934605...8cc7529. Read the comment docs.

@kristw kristw changed the title [refactor] Split visTypes into one file for each visualization type [reviewable] [refactor] Split visTypes into one file for each visualization type Nov 7, 2018
@kristw kristw closed this Nov 8, 2018
@kristw kristw reopened this Nov 8, 2018
@mistercrunch
Copy link
Member

LGTM

@@ -0,0 +1,47 @@
import { t } from '@superset-ui/translation';
import * as sections from './sections';
Copy link
Contributor

Choose a reason for hiding this comment

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

same re *

@@ -0,0 +1,52 @@
import { t } from '@superset-ui/translation';
import * as sections from './sections';
Copy link
Contributor

Choose a reason for hiding this comment

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

same re *

});
}

return [].concat(
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason not to use ... here since you use it for all object spreading?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Umm, this block of code I didn't touch. It was copy straight from original.
Prefer to leave it the same for now.

Copy link
Contributor

@williaster williaster left a comment

Choose a reason for hiding this comment

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

LGTM overall, couple small comments 👍

@williaster williaster merged commit 3d77526 into apache:master Nov 8, 2018
@kristw kristw deleted the kristw-control-panels branch November 8, 2018 22:11
bipinsoniguavus pushed a commit to ThalesGroup/incubator-superset that referenced this pull request Dec 26, 2018
…zation type (apache#6290)

* extract two files

* pass linting

* rename variables

* rename file

* fix lint

* Rename controlPanelConfigs.js to controlPanels/index.js

* use specific imports
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.34.0 labels Feb 27, 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 🚢 0.34.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants