feat: implement shared dashboard plugin wrapper #1672
Conversation
edoardo
force-pushed
the
feat/dashboard-plugin-wrapper
branch
from
70196cf
to
9f1d291
Compare
edoardo
force-pushed
the
feat/dashboard-plugin-wrapper
branch
from
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?) |
| // NB: Adding `startRecording` to dependencies causes | ||
| // an infinite recording loop as-is (probably need to memoize it) | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
I've left a comment in the platform-team Slack channel about this. This is a bug in the useCachebleSection hook as I see it.
| // Get & send PWA installation status now | ||
| getPWAInstallationStatus({ | ||
| onStateChange: onInstallationStatusChange, | ||
| }).then(onInstallationStatusChange) |
There was a problem hiding this comment.
I am a bit confused about these lines. I don't know why we have to pass the same function both as a parameter/option and then also call it ourselves. at first glance it appears to me that the .then() part is redundant and should be done by getPWAInstallationStatus.
| if (!navigator.serviceWorker) { | ||
| // Nothing to do here | ||
| return null | ||
| } | ||
|
|
||
| const registration = await navigator.serviceWorker.getRegistration() | ||
| if (!registration) { | ||
| // This shouldn't happen since this is a PWA app, but return null | ||
| return null | ||
| } | ||
|
|
||
| if (registration.active) { | ||
| return INSTALLATION_STATES.READY | ||
| } | ||
| // note that 'registration.waiting' is skipped - it implies there's an active one | ||
| if (registration.installing) { | ||
| handleInstallingWorker({ | ||
| installingWorker: registration.installing, | ||
| onStateChange, | ||
| }) | ||
| return INSTALLATION_STATES.INSTALLING | ||
| } |
There was a problem hiding this comment.
See my previous comment about the code in the useEffect hook. I guess you could say that this parts computes the initial state and I think we can add some calls to onStateChange here to avoid having to call then later on.
This takes care of offline caching for plugins which works the same in all analytics apps.
edoardo
force-pushed
the
feat/dashboard-plugin-wrapper
branch
from
7c8d2c8
to
87873cf
Compare
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
The Storybook build command fails.
It used to run automatically after the
buildcommand, 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-ciso it can be used in app's PRs.I think the issue is related to the upgrade of
cli-app-scriptswhich 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.