-
Notifications
You must be signed in to change notification settings - Fork 214
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
O3-2857 Add patient flag for active visits widget and dashboard link #1658
base: main
Are you sure you want to change the base?
Conversation
Size Change: +627 B (+0.01%) Total Size: 11.1 MB ℹ️ View Unchanged
|
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 didn't understand the flow "Current Visit" here?
The current visit has it's own summary tile, what is the point of creating a new tab for that?
As far as I can understand, the config point is to show action buttons in the summary and the forms filled in a visit as separate tabs.
...-chart-app/src/visit/visits-widget/current-visit-actions/current-visit-actions.component.tsx
Outdated
Show resolved
Hide resolved
...-chart-app/src/visit/visits-widget/current-visit-actions/current-visit-actions.component.tsx
Outdated
Show resolved
Hide resolved
...patient-chart-app/src/visit/visits-widget/past-visits-components/visit-summary.component.tsx
Outdated
Show resolved
Hide resolved
...patient-chart-app/src/visit/visits-widget/past-visits-components/visit-summary.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visits-widget/visit-detail-overview.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visits-widget/visit-detail-overview.component.tsx
Outdated
Show resolved
Hide resolved
The tab shows an easy way to access forms filled during a visit and shortcuts to commonly used forms/widgets |
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.
Thanks for the PR:
- I think this PR does too much. It should be broken down into multiple PRs (i.e., the current visit buttons make sense outside of the scope of the form-rendering stuff).
- The form-rendering stuff is not mergable into the RefApp as it. We can put it behind a feature flag, but it cannot be part of the RefApp until we can switch from the Angular form engine to the React form engine.
- I'm not really convinced that a per-form tab is the right way to go rather than, say, a "Forms" tab that can give you each encounter as a form.
- Note that there are non-form encounters that this may render completely inaccessible to users.
...-chart-app/src/visit/visits-widget/current-visit-actions/current-visit-actions.component.tsx
Outdated
Show resolved
Hide resolved
@@ -0,0 +1,8 @@ | |||
@use '@carbon/styles/scss/type'; |
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.
We don't need this.
> | ||
{t('encounters_title', 'Encounters')} | ||
</Tab> | ||
{showActiveVisitTab ? ( |
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.
showActiveVisitTab
seems like a poorly-named configuration item. It's something like "replaceEncounterTabWithPerFormTab"?
{t('encounters_title', 'Encounters')} | ||
</Tab> | ||
{showActiveVisitTab ? ( | ||
uniqueFormNames?.length > 0 && |
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 length check here is redundant.
const [selectedTab, setSelectedTab] = useState(''); | ||
const [selectedIndex, setSelectedIndex] = useState(0); |
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.
Carbon's Tabs components should already handle this. Adding our own state on top of this is going to create a maintenance nightmare.
@@ -112,6 +117,21 @@ const VisitSummary: React.FC<VisitSummaryProps> = ({ visit, patientUuid }) => { | |||
}; | |||
}, [visit?.encounters]); | |||
|
|||
const uniqueFormNames = getUniqueFormNames(visit); | |||
const groupedEncounters = groupBy(visit.encounters, 'encounterType.uuid'); |
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 the assumption that form name === encounter type is not one we should make.
groupedEncounters[selectedTab]?.map((val: any, ind) => | ||
val?.form && ind + 3 === selectedIndex ? ( | ||
<TabPanel> | ||
<OHRIForm patientUUID={patientUuid} formUUID={val.form?.uuid} encounterUUID={val.uuid} mode="view" /> |
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't go into the RefApp; until the RefApp can switch over to the React Form Engine, depending on it is a non-starter.
Thanks @ibacher and @vasharma05 for the comments and insights. I will make use of the extensions for now until we can switch from the Angular form engine to the React form engine. Closing this . |
f935476
to
f9d8f61
Compare
packages/esm-patient-chart-app/src/visit/visits-widget/active-visits-summary.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visits-widget/active-visits-summary.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-chart-app/src/visit/visits-widget/active-visits-summary.component.tsx
Outdated
Show resolved
Hide resolved
...patient-chart-app/src/visit/visits-widget/past-visits-components/visit-summary.component.tsx
Outdated
Show resolved
Hide resolved
...patient-chart-app/src/visit/visits-widget/past-visits-components/visit-summary.component.tsx
Outdated
Show resolved
Hide resolved
f9d8f61
to
a537c40
Compare
@CynthiaKamau: Am trying to understand how the different sections i.e. lab tests, notes, and prescriptions will be used considering that a provider should access the full list of these irrespective of the visits. My understanding is that a malaria test in visit 1 should guide a provider handling visit 2 not to order that same test, and the same should/may apply to drugs and/or notes. What are the actual use cases for this? Sorry if am off the point. |
a537c40
to
675d4fe
Compare
Since sections like lab tests, notes, and prescriptions are service agnostic, they are not changing, the history will still be available the same way it was before, only the encounters section will be broken down to display forms filled and have shortcuts to workspaces that may need to accessed in a visit, reducing the number of clicks |
b2ce90f
to
be5a63c
Compare
@vasharma05 or @ibacher can I ask you to please re-review this so Cynthia can close this out? (Since you originally reviewed it.) |
@CynthiaKamau I'm seeing no new comments since my last comment 2 weeks ago. I suggest that you work on resolving or responding to the unresponded to comments left by Ian at the top and then update here when ready for re-review :) How does that sound? Am I missing something? |
I have addresses majority of the comments and thee feature is now configurable |
Thanks @gracepotma , i think majority of the suggestions have been addressed. The feature is configurable and this will address limitation on the RFE. |
c972f4d
to
ff4fdc5
Compare
Hi @CynthiaKamau how does this play into the designs Paul is working on? Would it be better to first harmonize this current design with it so we don't do double work? |
Thats work in progress, there is a talk post for it. |
75a4ea9
to
0d1785e
Compare
Hi @CynthiaKamau , if there's a Talk thread you are referring to, please provide the link. Otherwise no one knows what you're talking about. Based on the fact that you've made commits since then it seems like you want to go ahead with this PR. So I'll ask @ibacher, please take another look when you have the chance. |
0d1785e
to
0986f1c
Compare
0986f1c
to
8c77bed
Compare
I hope there are plans to merge this into the existing visits widget. |
Requirements
Summary
Screenshots
Active.visit.20_02_2024.mov
Related Issue
Other