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.

Sign up for GitHub

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: fixed dimensions efficiency #1386

Merged
merged 6 commits into from
Sep 10, 2024
Merged

feat: fixed dimensions efficiency #1386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b574425
feat: fixed dimensions efficiency
tomzemp Aug 30, 2024
5868050
feat: pr feedback
tomzemp Sep 6, 2024
737b238
fix: move height/width out of styles
tomzemp Sep 9, 2024
f8b5d98
fix: update prop types
tomzemp Sep 10, 2024
711a520
fix: update upload-artifact
tomzemp Sep 10, 2024
722da5f
fix: update download-artifact
tomzemp Sep 10, 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
8 changes: 4 additions & 4 deletions .github/workflows/dhis2-verify-lib.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ jobs:
- name: Build
run: yarn build

- uses: actions/upload-artifact@v2
- uses: actions/upload-artifact@v3
with:
name: lib-build
path: |
Expand All @@ -77,7 +77,7 @@ jobs:
path: '**/node_modules'
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}

- uses: actions/download-artifact@v2
- uses: actions/download-artifact@v3
with:
name: lib-build

Expand All @@ -104,7 +104,7 @@ jobs:
path: '**/node_modules'
key: ${{ runner.os }}-yarn-${{ hashFiles('**/yarn.lock') }}

- uses: actions/download-artifact@v2
- uses: actions/download-artifact@v3
with:
name: lib-build

Expand All @@ -123,7 +123,7 @@ jobs:
with:
node-version: 14.x

- uses: actions/download-artifact@v2
- uses: actions/download-artifact@v3
with:
name: lib-build

Expand Down
16 changes: 8 additions & 8 deletions docs/components/Plugin.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,14 +51,14 @@ const MyPlugin = (propsFromParent) => {

## Plugin Props (reserved props)

| Name | Type | Required | Description |
| :--------------------: | :------------: | :---------------------------------------------: | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| **pluginShortName** | _string_ | _required_ if `pluginSource` is not provided | The shortName of the app/plugin you wish to load (matching the result from api/apps). Used to look up the plugin entry point. If this is not provided, `pluginSource` must be provided. `pluginSource` will take precedence if provided. |
| **pluginSource** | _string_ (url) | _required_ if `pluginShortName` is not provided | The URL of the plugin. If this is not provided, `pluginShortName` must be provided. |
| **onError** | _Function_ | _optional_ | Callback function to be called when an error in the plugin triggers an error boundary. You can use this to pass an error back up to the app and create a custom handling/UX if errors occur in the plugin. In general, it is recommended that you use the plugin's built-in error boundaries |
| **showAlertsInPlugin** | _boolean_ | _optional_ | If `true`, any alerts within the plugin (defined with the `useAlert` hook) will be rendered within the iframe. By default, this is `false`. It is recommended, in general, that you do not override this and allow alerts to be hoisted up to the app level |
| **height** | _number_ | _optional_ | If a height is provided, the iframe will be fixed to the specified height. If no height is provided, the iframe will automatically resize based on its contents. |
| **width** | _number_ | _optional_ | If a width is provided, the iframe will be fixed to the specified width. If no width is provided, the iframe will automatically resize based on its contents. |
| Name | Type | Required | Description |
| :--------------------: | :------------: | :---------------------------------------------: | --------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
| **pluginShortName** | _string_ | _required_ if `pluginSource` is not provided | The shortName of the app/plugin you wish to load (matching the result from api/apps). Used to look up the plugin entry point. If this is not provided, `pluginSource` must be provided. `pluginSource` will take precedence if provided. |
| **pluginSource** | _string_ (url) | _required_ if `pluginShortName` is not provided | The URL of the plugin. If this is not provided, `pluginShortName` must be provided. |
| **onError** | _Function_ | _optional_ | Callback function to be called when an error in the plugin triggers an error boundary. You can use this to pass an error back up to the app and create a custom handling/UX if errors occur in the plugin. In general, it is recommended that you use the plugin's built-in error boundaries |
| **showAlertsInPlugin** | _boolean_ | _optional_ | If `true`, any alerts within the plugin (defined with the `useAlert` hook) will be rendered within the iframe. By default, this is `false`. It is recommended, in general, that you do not override this and allow alerts to be hoisted up to the app level |
| **height** | _string_ | _optional_ | If a height is provided, the iframe will be fixed to the specified height. If no height is provided, the iframe will automatically resize its height based on its contents. The value of `height` will not be passed to the plugin, as it is in an internal implementation detail. If you do need to also pass the height to the plugin, you can pass another variable (e.g. `pluginHeight`). |
| **width** | _string_ | _optional_ | If a width is provided, the iframe will be fixed to the specified width. If no width is provided, the iframe will automatically resize its width based on its contents. The value of `width` will not be passed to the plugin, as it is in an internal implementation detail. If you do need to also pass the width to the plugin, you can pass another variable (e.g. `pluginWidth`). |

## Plugin Props (custom props)

Expand Down
32 changes: 16 additions & 16 deletions examples/cra/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1054,32 +1054,32 @@
integrity sha512-ij4wRiunFfaJxjB0BdrYHIH8FxBJpOwNPhhAcunlmPdXudL1WQV1qoP9un6JsEBAgQH+7UXyyjh0g7jTxXK6tg==

"@dhis2/app-runtime@file:../../runtime":
version "3.10.4"
version "3.10.6"
dependencies:
"@dhis2/app-service-alerts" "3.10.4"
"@dhis2/app-service-config" "3.10.4"
"@dhis2/app-service-data" "3.10.4"
"@dhis2/app-service-offline" "3.10.4"
"@dhis2/app-service-plugin" "3.10.4"
"@dhis2/app-service-alerts" "3.10.6"
"@dhis2/app-service-config" "3.10.6"
"@dhis2/app-service-data" "3.10.6"
"@dhis2/app-service-offline" "3.10.6"
"@dhis2/app-service-plugin" "3.10.6"

"@dhis2/app-service-alerts@3.10.4", "@dhis2/app-service-alerts@file:../../services/alerts":
version "3.10.4"
"@dhis2/app-service-alerts@3.10.6", "@dhis2/app-service-alerts@file:../../services/alerts":
version "3.10.6"

"@dhis2/app-service-config@3.10.4", "@dhis2/app-service-config@file:../../services/config":
version "3.10.4"
"@dhis2/app-service-config@3.10.6", "@dhis2/app-service-config@file:../../services/config":
version "3.10.6"

"@dhis2/app-service-data@3.10.4", "@dhis2/app-service-data@file:../../services/data":
version "3.10.4"
"@dhis2/app-service-data@3.10.6", "@dhis2/app-service-data@file:../../services/data":
version "3.10.6"
dependencies:
react-query "^3.13.11"

"@dhis2/app-service-offline@3.10.4", "@dhis2/app-service-offline@file:../../services/offline":
version "3.10.4"
"@dhis2/app-service-offline@3.10.6", "@dhis2/app-service-offline@file:../../services/offline":
version "3.10.6"
dependencies:
lodash "^4.17.21"

"@dhis2/app-service-plugin@3.10.4", "@dhis2/app-service-plugin@file:../../services/plugin":
version "3.10.4"
"@dhis2/app-service-plugin@3.10.6", "@dhis2/app-service-plugin@file:../../services/plugin":
version "3.10.6"
dependencies:
post-robot "^10.0.46"

Expand Down
32 changes: 16 additions & 16 deletions examples/query-playground/yarn.lock
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1812,32 +1812,32 @@
moment "^2.24.0"

"@dhis2/app-runtime@*", "@dhis2/app-runtime@^2.8.0", "@dhis2/app-runtime@file:../../runtime":
version "3.10.4"
version "3.10.6"
dependencies:
"@dhis2/app-service-alerts" "3.10.4"
"@dhis2/app-service-config" "3.10.4"
"@dhis2/app-service-data" "3.10.4"
"@dhis2/app-service-offline" "3.10.4"
"@dhis2/app-service-plugin" "3.10.4"
"@dhis2/app-service-alerts" "3.10.6"
"@dhis2/app-service-config" "3.10.6"
"@dhis2/app-service-data" "3.10.6"
"@dhis2/app-service-offline" "3.10.6"
"@dhis2/app-service-plugin" "3.10.6"

"@dhis2/app-service-alerts@3.10.4", "@dhis2/app-service-alerts@file:../../services/alerts":
version "3.10.4"
"@dhis2/app-service-alerts@3.10.6", "@dhis2/app-service-alerts@file:../../services/alerts":
version "3.10.6"

"@dhis2/app-service-config@3.10.4", "@dhis2/app-service-config@file:../../services/config":
version "3.10.4"
"@dhis2/app-service-config@3.10.6", "@dhis2/app-service-config@file:../../services/config":
version "3.10.6"

"@dhis2/app-service-data@3.10.4", "@dhis2/app-service-data@file:../../services/data":
version "3.10.4"
"@dhis2/app-service-data@3.10.6", "@dhis2/app-service-data@file:../../services/data":
version "3.10.6"
dependencies:
react-query "^3.13.11"

"@dhis2/app-service-offline@3.10.4", "@dhis2/app-service-offline@file:../../services/offline":
version "3.10.4"
"@dhis2/app-service-offline@3.10.6", "@dhis2/app-service-offline@file:../../services/offline":
version "3.10.6"
dependencies:
lodash "^4.17.21"

"@dhis2/app-service-plugin@3.10.4", "@dhis2/app-service-plugin@file:../../services/plugin":
version "3.10.4"
"@dhis2/app-service-plugin@3.10.6", "@dhis2/app-service-plugin@file:../../services/plugin":
version "3.10.6"
dependencies:
post-robot "^10.0.46"

Expand Down
41 changes: 14 additions & 27 deletions services/plugin/src/Plugin.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,23 +27,18 @@ const getPluginEntryPoint = ({
export const Plugin = ({
pluginSource,
pluginShortName,
height,
width,
...propsToPassNonMemoized
}: {
pluginSource?: string
pluginShortName?: string
height?: string | number
width?: string | number
propsToPass: any
}): JSX.Element => {
const iframeRef = useRef<HTMLIFrameElement>(null)

// we do not know what is being sent in passed props, so for stable reference, memoize using JSON representation
const propsToPassNonMemoizedJSON = JSON.stringify(propsToPassNonMemoized)
const propsToPass: any = useMemo(
Copy link
Member Author

Choose a reason for hiding this comment

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

I removed this as I think it was unnecessary extra memoization, though it's not strictly related to the goal of the ticket.

() => ({ ...propsToPassNonMemoized }),
// eslint-disable-next-line react-hooks/exhaustive-deps
[propsToPassNonMemoizedJSON]
)
const { height, width } = propsToPass

const { add: alertsAdd } = useContext(AlertsManagerContext)

const { data } = useDataQuery(appsInfoQuery)
Expand All @@ -60,26 +55,18 @@ export const Plugin = ({
useState<boolean>(false)

const [inErrorState, setInErrorState] = useState<boolean>(false)
const [pluginHeight, setPluginHeight] = useState<number>(150)
const [pluginWidth, setPluginWidth] = useState<number>(500)

useEffect(() => {
if (height) {
setPluginHeight(height)
}
if (width) {
setPluginWidth(width)
}
}, [height, width])
const [resizedHeight, setPluginHeight] = useState<number>(150)
const [resizedWidth, setPluginWidth] = useState<number>(500)

// since we do not know what props are passed, the dependency array has to be keys of whatever is standard prop
// we exclude height/width from memoization to avoid updates for these properties
const memoizedPropsToPass = useMemo(
() => propsToPass,
() => propsToPassNonMemoized,
/* eslint-disable react-hooks/exhaustive-deps */
[
...Object.keys(propsToPass)
...Object.keys(propsToPassNonMemoized)
.sort()
.map((k) => (propsToPass as any)[k]),
.map((k) => (propsToPassNonMemoized as any)[k]),
]
/* eslint-enable react-hooks/exhaustive-deps */
)
Expand All @@ -100,8 +87,8 @@ export const Plugin = ({
const iframeProps = {
...memoizedPropsToPass,
alertsAdd,
setPluginHeight,
setPluginWidth,
setPluginHeight: !height ? setPluginHeight : null,
setPluginWidth: !width ? setPluginWidth : null,
setInErrorState,
setCommunicationReceived,
}
Expand Down Expand Up @@ -156,9 +143,9 @@ export const Plugin = ({
<iframe
ref={iframeRef}
src={pluginSource}
width={width ?? resizedWidth + 'px'}
height={height ?? resizedHeight + 'px'}
style={{
width: `${pluginWidth}px`,
height: `${pluginHeight}px`,
border: 'none',
}}
></iframe>
Expand Down
Loading