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

feat: implement shared dashboard plugin wrapper #1672

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
4bfe16f
feat: implement dashboard plugin wrapper component
edoardo Jun 3, 2024
b15a1ab
chore: bump app-runtime and cli-app-scripts dev dependencies
edoardo Jun 3, 2024
f0ec2ad
chore: bump cli-app-scripts
edoardo Jun 7, 2024
3327723
chore: deduplicate yarn.lock
edoardo Jun 7, 2024
5672b60
fix: only use cache if plugin has PWA enabled
edoardo Jun 19, 2024
f1fdd5f
chore: remove postbuild temporarily
edoardo Jun 19, 2024
e9252d3
fix: remove required props and add defaults
edoardo Jun 20, 2024
29a2c6f
chore: tweak ESLint config so that jsx property is allowed
HendrikThePendric Jul 2, 2024
b7e4c16
chore: fix import order ESLint errors
HendrikThePendric Jul 2, 2024
af456a2
chore: use named functions for default exports too
HendrikThePendric Jul 2, 2024
1e9dda2
refactor: import ui components from the root package
edoardo Aug 6, 2024
cd99b54
fix: remove eslint disable and fix useEffect dep list
edoardo Aug 9, 2024
292f88f
chore: bump app-runtime to latest
edoardo Aug 9, 2024
e55b82f
Merge remote-tracking branch 'origin/master' into feat/dashboard-plug…
edoardo Aug 27, 2024
6b3e34a
Merge branch 'master' into feat/dashboard-plugin-wrapper
edoardo Sep 20, 2024
cfa7893
Merge branch 'master' into feat/dashboard-plugin-wrapper
edoardo Oct 10, 2024
bb4b556
chore: regenerate yarn.lock
edoardo Oct 9, 2024
d24d959
Merge remote-tracking branch 'origin/master' into feat/dashboard-plug…
edoardo Oct 30, 2024
47c1be0
fix: use correct function arguments
edoardo Oct 30, 2024
6cbd380
Merge remote-tracking branch 'origin/master' into feat/dashboard-plug…
edoardo Nov 11, 2024
8bf4ac6
refactor: import ui components from the root package
edoardo Nov 11, 2024
1b6dda6
Merge remote-tracking branch 'origin/master' into feat/dashboard-plug…
edoardo Nov 15, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
"validate-push": "yarn test"
},
"devDependencies": {
"@dhis2/app-runtime": "^3.9.0",
"@dhis2/app-runtime": "^3.10.6",
"@dhis2/cli-app-scripts": "^11.7.4",
"@dhis2/cli-style": "^10.7.4",
"@dhis2/d2-i18n": "^1.1.0",
Expand Down
96 changes: 96 additions & 0 deletions src/components/DashboardPluginWrapper/DashboardPluginWrapper.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import {
useCacheableSection,
CacheableSection,
useConfig,
} from '@dhis2/app-runtime'
import { CenteredContent, CircularLoader, CssVariables, Layer } from '@dhis2/ui'
import PropTypes from 'prop-types'
import React, { useEffect } from 'react'
import { getPWAInstallationStatus } from '../../modules/getPWAInstallationStatus.js'

const LoadingMask = () => {
return (
<Layer>
<CenteredContent>
<CircularLoader />
</CenteredContent>
</Layer>
)
}

const CacheableSectionWrapper = ({ id, children, isParentCached }) => {
const { startRecording, isCached, remove } = useCacheableSection(id)

useEffect(() => {
if (isParentCached && !isCached) {
startRecording({ onError: console.error })
} else if (!isParentCached && isCached) {
// Synchronize cache state on load or prop update
// -- a back-up to imperative `removeCachedData`
remove()
}
}, [isCached, isParentCached, remove, startRecording])

return (
<CacheableSection id={id} loadingMask={<LoadingMask />}>
{children}
</CacheableSection>
)
}

CacheableSectionWrapper.propTypes = {
children: PropTypes.node,
id: PropTypes.string,
isParentCached: PropTypes.bool,
}

export const DashboardPluginWrapper = ({
onInstallationStatusChange,
children,
cacheId,
isParentCached,
...props
}) => {
const { pwaEnabled } = useConfig()

useEffect(() => {
// Get & send PWA installation status now
getPWAInstallationStatus({
onStateChange: onInstallationStatusChange,
}).then(onInstallationStatusChange)
Comment on lines +57 to +60
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@KaiVandivier KaiVandivier Aug 8, 2024

Choose a reason for hiding this comment

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

I did it this way to communicate that you can definitely expect a value "now" (-ish; the service worker registration API is asynchronous), and you may get another one or two updates later -- maybe that didn't end up being obvious though

The current design also has two small benefits that I can see, though they're currently not taken advantage of:

  1. A different handler can be used for the initial state value received than for the subsequent updates
  2. This structure can be easily awaited if you want to control the flow when rendering the plugin for the first time

Your suggested refactor in another comment would certainly work though, and you guys are welcome to implement it if you prefer 👍 it should be pretty simple: adding onStateChange(<state>) before any return <state>

}, [onInstallationStatusChange])

return props ? (
<div
style={{
display: 'flex',
height: '100%',
overflow: 'hidden',
}}
>
{pwaEnabled ? (
<CacheableSectionWrapper
id={cacheId}
isParentCached={isParentCached}
>
{children(props)}
</CacheableSectionWrapper>
) : (
children(props)
)}
<CssVariables colors spacers elevations />
</div>
) : null
}

DashboardPluginWrapper.defaultProps = {
isParentCached: false,
onInstallationStatusChange: Function.prototype,
}

DashboardPluginWrapper.propTypes = {
cacheId: PropTypes.string,
children: PropTypes.func,
isParentCached: PropTypes.bool,
onInstallationStatusChange: PropTypes.func,
}
3 changes: 1 addition & 2 deletions src/components/Toolbar/HoverMenuBar/HoverMenuDropdown.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { Popper } from '@dhis2-ui/popper'
import { Portal } from '@dhis2-ui/portal'
import { Popper, Portal } from '@dhis2/ui'
import cx from 'classnames'
import PropTypes from 'prop-types'
import React, { useRef } from 'react'
Expand Down
2 changes: 1 addition & 1 deletion src/components/Toolbar/HoverMenuBar/HoverMenuList.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { colors, elevations, spacers } from '@dhis2/ui-constants'
import { colors, elevations, spacers } from '@dhis2/ui'
import PropTypes from 'prop-types'
import React, { createContext, useCallback, useContext, useState } from 'react'
import { useHoverMenubarContext } from './HoverMenuBar.js'
Expand Down
4 changes: 1 addition & 3 deletions src/components/Toolbar/HoverMenuBar/HoverMenuListItem.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
import { IconChevronRight24 } from '@dhis2/ui-icons'
import { Popper } from '@dhis2-ui/popper'
import { Portal } from '@dhis2-ui/portal'
import { IconChevronRight24, Popper, Portal } from '@dhis2/ui'
import cx from 'classnames'
import PropTypes from 'prop-types'
import React, { useRef } from 'react'
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { colors, spacers } from '@dhis2/ui-constants'
import { colors, spacers } from '@dhis2/ui'
import css from 'styled-jsx/css'

export default css`
Expand Down
2 changes: 1 addition & 1 deletion src/components/Toolbar/MenuButton.styles.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { colors, spacers, theme } from '@dhis2/ui-constants'
import { colors, spacers, theme } from '@dhis2/ui'
import css from 'styled-jsx/css'

export default css`
Expand Down
2 changes: 1 addition & 1 deletion src/components/Toolbar/Toolbar.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { colors } from '@dhis2/ui-constants'
import { colors } from '@dhis2/ui'
import PropTypes from 'prop-types'
import React from 'react'

Expand Down
2 changes: 1 addition & 1 deletion src/components/Toolbar/ToolbarSidebar.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { colors } from '@dhis2/ui-constants'
import { colors } from '@dhis2/ui'
import cx from 'classnames'
import PropTypes from 'prop-types'
import React from 'react'
Expand Down
4 changes: 1 addition & 3 deletions src/components/Toolbar/UpdateButton.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
import i18n from '@dhis2/d2-i18n'
import { colors } from '@dhis2/ui-constants'
import { IconSync16 } from '@dhis2/ui-icons'
import { CircularLoader } from '@dhis2-ui/loader'
import { CircularLoader, IconSync16, colors } from '@dhis2/ui'
import PropTypes from 'prop-types'
import React from 'react'
import menuButtonStyles from './MenuButton.styles.js'
Expand Down
2 changes: 2 additions & 0 deletions src/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ export {

export * from './components/RichText/index.js'

export { DashboardPluginWrapper } from './components/DashboardPluginWrapper/DashboardPluginWrapper.js'

// Api

export { default as Analytics } from './api/analytics/Analytics.js'
Expand Down
63 changes: 63 additions & 0 deletions src/modules/getPWAInstallationStatus.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
export const INSTALLATION_STATES = {
READY: 'READY',
INSTALLING: 'INSTALLING',
}

function handleInstallingWorker({ installingWorker, onStateChange }) {
installingWorker.onstatechange = () => {
if (installingWorker.state === 'activated') {
// ... and update state to 'ready'
onStateChange(INSTALLATION_STATES.READY)
}
}
}

/**
* Gets the current installation state of the PWA features, which is intended
* to be reported from this plugin to the parent app to indicate that the
* static assets are cached and ready to be accessed locally instead of over
* the network.
*
* Returns either READY, INSTALLING, or `null` for not installed/won't install
*/
export async function getPWAInstallationStatus({ onStateChange }) {
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
}
Comment on lines +24 to +45
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@KaiVandivier do you have any reply to these comments?
I believe was you that wrote this part originally.


// It shouldn't normally be possible to get here, but just in case,
// listen for installations
registration.onupdatefound = () => {
// update state for this plugin to 'installing'
onStateChange(INSTALLATION_STATES.INSTALLING)

// also listen for the installing worker to become active
const installingWorker = registration.installing
if (!installingWorker) {
return
}
handleInstallingWorker({ installingWorker, onStateChange })
}

// and in the mean time, return null to show 'not installed'
return null
}
Loading
Loading