-
Notifications
You must be signed in to change notification settings - Fork 14k
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
feat: Dynamic dashboard component #17208
feat: Dynamic dashboard component #17208
Conversation
� Conflicts: � superset-frontend/src/dashboard/util/getPermissions.ts
Codecov Report
@@ Coverage Diff @@
## master #17208 +/- ##
==========================================
- Coverage 66.31% 66.26% -0.05%
==========================================
Files 1595 1603 +8
Lines 62599 62715 +116
Branches 6297 6316 +19
==========================================
+ Hits 41513 41559 +46
- Misses 19440 19507 +67
- Partials 1646 1649 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
May be functional registry can be moved to
P.S. other stuff fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @simcha90, in order to local test, could you rebase this PR from the master branch? Thanks!
I got some lint errors in my local. I think if we rebase master might suppress this issue in IDE.
@zhaoyongjie done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code LGTM and in general I think the proposed solution looks great. However, to further decouple this and make it more approachable for plugin developers, I'd like to propose the following:
- add a Yeoman template, similar to the viz plugin one, to
superset-ui/generator-superset
. This would optimally generate the example component. By doing this we would not need the example component in the actual codebase - Add a section to the docs on how to create a dashboard plugin, something similar to https://superset.apache.org/docs/contributing/creating-viz
My long term vision is to keep making it easier to add plugins, hopefully removing the need for manually registering the plugins in the superset-frontend
codebase all together (I'm thinking moving more and more in the direction of dynamic plugins). To spread the pain, I'm happy to tackle the docs, and can even help out moving the example plugin to the yeoman generator if needed.
8db30ef
to
c2a0516
Compare
c2a0516
to
3108ae0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
SUMMARY
This PR is the implementation of SIP -76 #17210 - feat for dynamic loading of dashboard components
Why?
Dependent on product needs we want to take development of Superset to different directions.
Dashboard Components
can introduce specific needs of company requirements. So we wanted to do this feature more flexible in order to not affect other product visions :)What?
This PR add opportunity to add new
Dashboard Components
that can be configured in separate file by every superset instance and not affects main Superset functionality. Commonly needed types relating to the Dashboard are also moved into@superset-ui/core
to make it possible to fully decouple dashboard component plugin development from the Superset codebase.How?
We creating new dashboard components registry that can register new
Dashboard Components
in/src/setup/setupDasboardComponents.ts
. In a follow-up PR, this code will be moved into a Yeoman generator template.When we drag new component to dashboard we use
React.lazy
function to load code of componentNOTES:
superset-frontend/src/setup/setupDasboardComponents.ts
and it will add demo component to list of componentsBEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
Screen.Recording.2021-10-24.at.15.41.42.mov
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION