-
Notifications
You must be signed in to change notification settings - Fork 76
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
(fix) Fix misplaced metrics extension slot #191
Conversation
Could you attach a screenshot that confirms that this works as you expect? |
import { ExtensionSlot } from '@openmrs/esm-framework'; | ||
import React from 'react'; | ||
|
||
const PageMetrics: React.FC = () => { |
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.
Nit: I'd just call this Metrics
for consistency with the file name.
packages/esm-home-app/src/index.ts
Outdated
@@ -24,6 +24,8 @@ export const homeWidgetDbLink = getSyncLifecycle(createDashboardLink(dashboardMe | |||
|
|||
export const homeWidgetDashboard = getSyncLifecycle(homeWidgetDashboardComponent, options); | |||
|
|||
export const metricsSlot = getAsyncLifecycle(() => import('./metrics/metrics.component'), options); |
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.
This can be named better @denniskigen should we just call this metrics
,metricsContainer
or MetricsComponent
export const metricsSlot = getAsyncLifecycle(() => import('./metrics/metrics.component'), options); | |
export const metricsContainer = getSyncLifecycle(MetricsComponent, options); |
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.
This is resolved.
import { ExtensionSlot } from '@openmrs/esm-framework'; | ||
import React from 'react'; | ||
|
||
const PageMetrics: React.FC = () => { |
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 naming in the index should trickle down here
@@ -24,6 +24,8 @@ export const homeWidgetDbLink = getSyncLifecycle(createDashboardLink(dashboardMe | |||
|
|||
export const homeWidgetDashboard = getSyncLifecycle(homeWidgetDashboardComponent, options); | |||
|
|||
export const metrics = getAsyncLifecycle(() => import('./metrics/metrics.component'), options); |
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.
export const metrics = getAsyncLifecycle(() => import('./metrics/metrics.component'), options); | |
export const metrics = getSyncLifecycle(Metrics, options); |
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.
Add the import Metrics
from ./metrics/metrics.component
at the top of the file
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.
Isn't getAsyncLifecycle
better for app performance? Or does it have specific use cases? Reference.
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 think there's a bit of misalignment on when to use getAsyncLifecycle
and when to use getSyncLifecycle
homeWidgetDashboard
uses getSyncLifecycle
to load a component that is similar to metrics. Let's leave as is for now and will circle back.
Requirements
Summary
This PR is a follow up fix to an error introduces by this PR.
Screenshots
None.
Related Issue
None.
Other
None.