BREAKING: O3-2827: Appointments: Refactor (Part I) #971
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,14 +18,18 @@ | |
| "extract-translations": "turbo extract-translations -- --config ../../tools/i18next-parser.config.js" | ||
| }, | ||
| "dependencies": { | ||
| "@hookform/resolvers": "^3.3.1", | ||
| "classnames": "^2.3.2", | ||
| "react-hook-form": "^7.46.2", | ||
| "swr": "^2.0.1", | ||
| "xlsx": "^0.18.5", | ||
| "zod": "^3.22.2" | ||
| }, | ||
| "devDependencies": { | ||
| "@babel/core": "^7.11.6", | ||
| "@carbon/react": "~1.37.0", | ||
| "@openmrs/esm-framework": "next", | ||
| "@openmrs/esm-patient-common-lib": "next", | ||
|
There was a problem hiding this comment. Do we need this dependency? I don't see it used in any of the code marked as coming from the chart. There was a problem hiding this comment. I forget why I needed it, but it was because something wasn't working... potentially something I later removed, I will take it out and see if things still work. There was a problem hiding this comment. It's used in the appointment-form.component @ibacher
|
||
| "@playwright/test": "1.40.1", | ||
| "@swc/core": "^1.2.165", | ||
| "@swc/jest": "^0.2.29", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -5,28 +5,28 @@ import utc from 'dayjs/plugin/utc'; | |
| import { Button, OverflowMenu, OverflowMenuItem } from '@carbon/react'; | ||
| import { TaskComplete } from '@carbon/react/icons'; | ||
| import { useTranslation } from 'react-i18next'; | ||
| import { closeOverlay, launchOverlay } from '../../hooks/useOverlay'; | ||
| import { type Appointment } from '../../types'; | ||
| import { showModal } from '@openmrs/esm-framework'; | ||
| import { useVisits } from '../../hooks/useVisits'; | ||
| import AppointmentForm from '../../form/appointments-form.component'; | ||
| import CheckInButton from './checkin-button.component'; | ||
|
|
||
| dayjs.extend(utc); | ||
| dayjs.extend(isToday); | ||
|
|
||
| interface AppointmentActionsProps { | ||
| visits: Array<any>; | ||
| appointment: Appointment; | ||
| scheduleType: string; | ||
| mutate: () => void; | ||
| } | ||
|
|
||
| const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appointment, mutate }) => { | ||
|
There was a problem hiding this comment. We seem to be passing The PatientSearchContext is a pretty good example of how this can be used. There was a problem hiding this comment. Yeah, it was annoying me that I had to pass this all the way around. I can check out out PatientSearchContext. Another pattern I was seeing was using the useSWR call at multiple levels just to retrieve the mutate function, but that didn't seem correct, and seemed dependent on making sure you were asking for the same/correct key at multiple levels. There was a problem hiding this comment. In the interest of getting this PR merged without continuing to bloat, I will handle this as a separate ticket here: https://openmrs.atlassian.net/browse/O3-2849 There was a problem hiding this comment. You can always create a new hook that does your |
||
| const { t } = useTranslation(); | ||
| const { mutateVisit } = useVisits(); | ||
| const patientUuid = appointment.patient.uuid; | ||
| const visitDate = dayjs(appointment.startDateTime); | ||
| const isFutureAppointment = visitDate.isAfter(dayjs()); | ||
| const isTodayAppointment = visitDate.isToday(); | ||
| const hasActiveVisit = visits?.some((visit) => visit?.patient?.uuid === patientUuid && visit?.startDatetime); | ||
|
|
@@ -44,17 +44,12 @@ const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appoint | |
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * Renders the appropriate visit status button based on the current appointment state. | ||
| * @returns {JSX.Element} The rendered button. | ||
| */ | ||
| const renderVisitStatus = () => { | ||
| const checkedOutText = t('checkedOut', 'Checked out'); | ||
|
|
||
| switch (true) { | ||
| case hasCheckedOut: | ||
|
|
@@ -70,27 +65,10 @@ const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appoint | |
| </Button> | ||
| ); | ||
| case isTodayAppointment: { | ||
|
Comment on lines
-73
to
-82
There was a problem hiding this comment. I'm not in a position to say whether this is right or not, but this does seem to indicate a point where we have implementation needs for extensibility that's not currently provided. |
||
| return <CheckInButton patientUuid={patientUuid} appointment={appointment} />; | ||
| } | ||
|
|
||
| default: | ||
| return null; | ||
| } | ||
| }; | ||
|
|
@@ -105,7 +83,12 @@ const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appoint | |
| onClick={() => | ||
| launchOverlay( | ||
| t('editAppointments', 'Edit Appointment'), | ||
| <AppointmentForm | ||
| appointment={appointment} | ||
| context="editing" | ||
| closeWorkspace={closeOverlay} | ||
| mutate={mutate} | ||
| />, | ||
| ) | ||
| } | ||
| /> | ||
|
|
||
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.
While this isn't terribly important, it would be preferrable, I think, to keep all dependencies in
devDependencies. These aren't in any way different from how we handle, e.g.,dayjs.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 @ibacher ! I thought I was just keeping this consistent with the patient chart, but it looks like they are all devDependencies there, I will correct.
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.
Okay, so looking again, I see that these dependencies are listed as dependencies, not devDependencies, in patient-chart. However, when I moved them all to devDependencies here things worked. Do we have a best practice regarding this (to be honest, I don't really understand why these are all devdepencies, is the assumption thst they are all bundled with ESM framework anyway)? @ibacher @denniskigen @brandones
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.
Yeah we are somewhat inconsistent on this because we changed the norm a couple times during the first couple years of O3. I don't think there is a technical difference between
devDependenciesanddependenciesfor us, with our current tooling and setup, though at least some monorepo tools installdevDependenciesat the monorepo level anddependenciesat the package level—but I think Yarn 4 just treats them the same.Oddly, my inclination would be the opposite of what @ibacher suggests here—I am inclined to follow the rule that a dependency that gets built/bundled goes in
dependencies. But I am quite open to being convinced otherwise 🤷