-
Notifications
You must be signed in to change notification settings - Fork 12
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: implement shared dashboard plugin wrapper #1672
Conversation
70196cf
to
9f1d291
Compare
aeb6161
to
08b5000
Compare
Since I don't think we actually use the storybook build anywhere, I think it is fine to not include a fix for it in this PR. We should probably create a new JIRA ticket with a slightly larger scope: both building the storybook, and then also publishing it somewhere (perhaps a netlify deploy?) |
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.
I've left some comments and am not sure if we should perhaps not merge this PR until that bug in the useCacheableSection
hook is solved. But will approve for logistical reasons.
src/components/DashboardPluginWrapper/DashboardPluginWrapper.js
Outdated
Show resolved
Hide resolved
7c8d2c8
to
87873cf
Compare
87873cf
to
1615bb4
Compare
This takes care of offline caching for plugins which works the same in all analytics apps.
1a5c9ef
to
292f88f
Compare
I created a task for this so we (hopefully) don't forget. This has been done and merged already, see PR #1724 . |
Remove resolution too.
Superceded by #1748 because of unfortunate dependency downgrades that were introduced due to regenerating the yarn.lock file. |
Relates to dhis2/data-visualizer-app#3082
Relates to dhis2/line-listing-app#396
Relates to dhis2/maps-app#3232
Key features
Description
This wrapper adds support for dashboard offline caching.
Before the logic was replicated in the plugin component of each analytics app.
Now all apps can use this wrapper and only implement their specific logic related to the props required to be passed down to the plugin rendering component.
Known issues
TheStorybookbuild command failshas been updated in a separate PR and works.It used to run automatically after the
build
command, so also in Github, which I'm not sure is necessary.It has been removed in the PR just to be able to build and copy the artifact on
d2-ci
so it can be used in app's PRs.I think the issue is related to the upgrade of
cli-app-scripts
which introduces Webpack 5, while the Storybook setup is for Webpack 4.I'm not sure an upgrade of Storybook belongs to this PR, but after this PR is merged, Storybook is broken.