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

chore: add shared plugin controls utilities #389

Merged
merged 9 commits into from Apr 28, 2020

Conversation

suddjian
Copy link
Member

💔 Breaking Changes

🏆 Enhancements

This moves several files from incubator-superset into @superset-ui/chart. These will allow moving the plugin controls config out of Superset's setupPlugins.ts and into the individual plugins for all the plugins. This covers the dependencies for all plugin controls except the DeckGL plugins. Those will come later.

The new modules are not exported directly by @superset-ui/chart, and must be imported by their directory paths under @superset-ui/chart/controls/[...]. This is to keep things organized and to avoid bloating the package size.

📜 Documentation

Added a paragraph to the readme. It's not enough documentation really, but things are very much in flux and I'd prefer to write more comprehensive docs once the plugin situation is more locked down.

🐛 Bug Fix

🏠 Internal

@suddjian suddjian requested a review from a team as a code owner April 22, 2020 23:58
@vercel
Copy link

vercel bot commented Apr 22, 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/5nom0vq4d
✅ Preview: https://superset-ui-git-fork-suddjian-chart-config-deps.superset.now.sh

@suddjian suddjian changed the title add shared controls utilities chore: add shared controls utilities Apr 22, 2020
@suddjian suddjian changed the title chore: add shared controls utilities chore: add shared plugin controls utilities Apr 22, 2020
@@ -0,0 +1,299 @@
/**
Copy link
Member

Choose a reason for hiding this comment

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

I think this could be a part of @superset-ui/legacy-preset-chart-nvd3 since it's written for (and only used by) that one plugin.

Same applies to the DeckGL equivalent - those shared controls could live with that plugin.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh thanks for the pointer, if it's only used there then that's way better.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @rusackas

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.

Maybe create a new pakage superset-ui/controls? It may also make sense to leave the NVD3 controls in incubator-superset for now. The way I see it, they are more like visualization-specific metadata that should go with the plugins.

// input choices & options
export const D3_FORMAT_OPTIONS = [
['SMART_NUMBER', 'Adaptative formating'],
[' ', 'Original value'],
Copy link
Contributor

Choose a reason for hiding this comment

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

This has been updated to "~g". See: apache/superset#9608

@codecov
Copy link

codecov bot commented Apr 24, 2020

Codecov Report

Merging #389 into master will increase coverage by 0.21%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #389      +/-   ##
==========================================
+ Coverage   26.31%   26.53%   +0.21%     
==========================================
  Files         192      196       +4     
  Lines        5293     5307      +14     
  Branches      475      475              
==========================================
+ Hits         1393     1408      +15     
  Misses       3864     3864              
+ Partials       36       35       -1     
Impacted Files Coverage Δ
...ges/superset-ui-chart/src/controls/D3Formatting.ts 100.00% <100.00%> (ø)
packages/superset-ui-chart/src/controls/index.ts 100.00% <100.00%> (ø)
...ckages/superset-ui-chart/src/controls/sections.tsx 100.00% <100.00%> (ø)
...es/superset-ui-chart/src/controls/selectOptions.ts 100.00% <100.00%> (ø)
packages/superset-ui-query/src/buildQueryObject.ts 100.00% <0.00%> (+7.69%) ⬆️

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 b1c93da...d5e70aa. 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, but we may want to get this one in first: apache/superset#9578

@kristw
Copy link
Contributor

kristw commented Apr 27, 2020

Maybe create a new package superset-ui/controls

Agree with @ktmud.

@@ -27,6 +27,8 @@
"access": "public"
},
"dependencies": {
"@superset-ui/translation": "^0.12.21",
Copy link
Contributor

Choose a reason for hiding this comment

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

These should be peerDependencies. (translation needs to be a singleton. validator not necessariliy, but I have been making all @superset-ui/xxx peerDependencies by convention to avoid having multiple copies of some singleton by mistake)

@rusackas
Copy link
Member

@kristw @ktmud I'm inclined to say this is OK as-is, and push it through for now. We should sync up about the plan for remaining controls and their dependencies. If they need to be in a new package (I'm not sure, but I'm open to it), we can address that in a separate PR. I would just want to call it something like controlUtils rather than controls since the latter sounds exactly like what we're all trying to kill off :)

@kristw kristw merged commit 2c3fe61 into apache-superset:master Apr 28, 2020
@suddjian suddjian deleted the chart-config-deps branch April 29, 2020 18:28
@ktmud
Copy link
Contributor

ktmud commented Jun 13, 2020

I would just want to call it something like controlUtils rather than controls since the latter sounds exactly like what we're all trying to kill off :)

@rusackas , can you clarify on this statement?

I was creating a new control type today (a radio button, to be specific) and thought it'd be beneficial to create it as a shared control.

A @superset-ui/controls or @superset-ui/chart-controls package seems a great place to store these shared control components, as well as those currently reside in superset-frontend/src/explore/component/controls. What are our plans regarding those?

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.

None yet

4 participants