BREAKING: O3-2827: Appointments: Refactor (Part I) #971
Conversation
# Conflicts: # packages/esm-appointments-app/src/home-appointments/appointments-list.component.tsx
# Conflicts: # packages/esm-appointments-app/src/appointments/forms/forms.resource.ts # packages/esm-appointments-app/src/home-appointments/appointments-list.test.tsx # packages/esm-appointments-app/src/home/appointments-base-table.component.tsx # packages/esm-appointments-app/src/home/home-appointments.resource.ts # packages/esm-appointments-app/src/patient-queue/visit-form/visit-form.component.tsx # packages/esm-appointments-app/src/root.component.tsx
|
Size Change: -290 kB (-9%) ✅ Total Size: 2.95 MB
ℹ️ View Unchanged
|
| @@ -0,0 +1,126 @@ | |||
| import dayjs from 'dayjs'; | |||
There was a problem hiding this comment.
Brought over from Patient Chart
| @@ -0,0 +1,105 @@ | |||
| @use '@carbon/styles/scss/colors'; | |||
There was a problem hiding this comment.
Brought over from Patient Chart
| @@ -0,0 +1,205 @@ | |||
| import React from 'react'; | |||
There was a problem hiding this comment.
Brought over from Patient Chart
mogoodrich
changed the title
|
This admittedly a rather large PR and therefore difficult to review, best to read the explanation I have written up here: |
mogoodrich
requested review from
donaldkibet,
CynthiaKamau,
denniskigen,
mseaton,
chibongho,
cioan and
ibacher
vasharma05
changed the title
|
per comment on the ticket, I will likely reincorporate the distribution calendar into the form before merging. |
There was a problem hiding this comment.
A few things I noticed, but mostly comments for future work. Personal bugbear of mine: the pattern of creating an abort controller inside a function call is basically useless. The right pattern for this is in the React component use our useAbortController() hook and pass that into any requests.
The point of abort controllers is to allow the application to cancel requests. If the abort controller's abort() method is never called, it doesn't do anything. useAbortController() calls the abort() method when the component is removed from the screen, so it's generally useful for aborting unnecessary GET requests.
| }, | ||
| "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
import { convertTime12to24 } from '@openmrs/esm-patient-common-lib';
| @@ -18,14 +18,18 @@ | |||
| "extract-translations": "turbo extract-translations -- --config ../../tools/i18next-parser.config.js" | |||
| }, | |||
| "dependencies": { | |||
| "@hookform/resolvers": "^3.3.1", | |||
There was a problem hiding this comment.
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.
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.
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.
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 devDependencies and dependencies for us, with our current tooling and setup, though at least some monorepo tools install devDependencies at the monorepo level and dependencies at 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 🤷
| const isAfterNoon = new Date().getHours() > 12; | ||
|
|
||
| if (scheduleType === 'Pending' && isAfterNoon) { | ||
| return ( | ||
| <Button onClick={handleOpenDefaulterForm} size="sm" kind="tertiary"> | ||
| {followUpButtonText} | ||
| </Button> | ||
| ); | ||
| } | ||
|
|
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.
| } | ||
|
|
||
| const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appointment, scheduleType }) => { | ||
| const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appointment, mutate }) => { |
There was a problem hiding this comment.
We seem to be passing mutate() through three or for different levels, just so we can access it as a prop in AppointmentForm. This seems like a perfect place for using a React context.
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 useSWR call. Then you are sure to be making the same one each time. You shouldn't be passing mutates around. If you don't like the aesthetic of using a fetch hook just to obtain the mutate, then you can also write a custom hook that just obtains the correct mutate function. See useMutatePatientOrders for example. Ignore the comment, it's out of date. PR
| @@ -130,7 +133,9 @@ const AppointmentsTable: React.FC<AppointmentsTableProps> = ({ | |||
| onClick={() => | |||
| downloadAppointmentsAsExcel( | |||
| appointments, | |||
| `${tableHeading} Appointments ${formatDate(new Date(appointments[0]?.dateTime), { year: true })}`, | |||
| `${tableHeading} Appointments ${formatDate(new Date(appointments[0]?.startDateTime), { | |||
There was a problem hiding this comment.
FIXME for latter, but this should all be translatable.
| const pastAppointments = appointments | ||
| ?.sort((a, b) => (b.startDateTime > a.startDateTime ? 1 : -1)) | ||
| ?.filter(({ status }) => status !== 'Cancelled') | ||
| ?.filter(({ startDateTime }) => | ||
| dayjs(new Date(startDateTime).toISOString()).isBefore(new Date().setHours(0, 0, 0, 0)), | ||
| ); | ||
|
|
||
| const upcomingAppointments = appointments | ||
| ?.sort((a, b) => (a.startDateTime > b.startDateTime ? 1 : -1)) | ||
| ?.filter(({ status }) => status !== 'Cancelled') | ||
| ?.filter(({ startDateTime }) => dayjs(new Date(startDateTime).toISOString()).isAfter(new Date())); | ||
|
|
||
| const todaysAppointments = appointments | ||
| ?.sort((a, b) => (a.startDateTime > b.startDateTime ? 1 : -1)) | ||
| ?.filter(({ status }) => status !== 'Cancelled') | ||
| ?.filter(({ startDateTime }) => dayjs(new Date(startDateTime).toISOString()).isToday()); |
There was a problem hiding this comment.
This seems refactorable:
const activeAppointments = appointments
?.filter(({ status }) => status !== 'Cancelled') ?? [];
// no ? here since `activeAppointments` is guaranteed to be an array
const pastAppointments = activeAppointments
.sort((a, b) => (b.startDateTime > a.startDateTime ? 1 : -1))
.filter(({ startDateTime }) =>
dayjs(new Date(startDateTime).toISOString()).isBefore(new Date().setHours(0, 0, 0, 0)),
);Not essential.
There was a problem hiding this comment.
Will take a look, just copied this code over from patient chart so hesitant to refactor unless I can clearly test.
There was a problem hiding this comment.
Oh, yes, this makes total sense @ibacher . However :) I'm not sure where this code is currently being used, so out of an abundance of caution I'm not going to refactor something I'm not 100% sure how to test. I suspect this will come over when I refactor the patient appointments to use this, instead of the version in the patient chart.
| export function getAppointmentsByUuid(appointmentUuid: string, abortController: AbortController) { | ||
| return openmrsFetch(`/ws/rest/v1/appointments/${appointmentUuid}`, { | ||
| signal: abortController.signal, | ||
| }); | ||
| } | ||
|
|
||
| export function getAppointmentService(abortController: AbortController, uuid) { | ||
| return openmrsFetch(`/ws/rest/v1/appointmentService?uuid=` + uuid, { | ||
| signal: abortController.signal, | ||
| }); | ||
| } |
There was a problem hiding this comment.
I haven't tracked the context in which these are used, but can they be re-written to take advantage of SWR?
There was a problem hiding this comment.
Code I copied over, I'm not sure where they are used as well... I hope to do a more thorough cleanup of all the resource calls in the future, I'll had a TODO to the code to take a look at that then.
| }); | ||
| } | ||
|
|
||
| export const cancelAppointment = async (toStatus: string, appointmentUuid: string) => { |
There was a problem hiding this comment.
Probably not this PR, but a general comment: If the only point at which you have an await in a function body is in the return, there is no point in marking it async.
(toStatus: string, appointmentUuid: string) => {
// ... snip ...
return openmrsFetch(/*... snip ...*/);
}Is precisely equivalent and takes less typing.
| @@ -51,7 +51,7 @@ export function startupApp() { | |||
| }, | |||
| { | |||
| path: `${window.spaBase}/patient-list/:forDate/:serviceName`, | |||
| title: ([date, serviceName]) => `Patient Lists / ${decodeURI(serviceName)}`, | |||
| title: ([serviceName]) => `Patient Lists / ${decodeURI(serviceName)}`, | |||
There was a problem hiding this comment.
Shouldn't this be:
| title: ([serviceName]) => `Patient Lists / ${decodeURI(serviceName)}`, | |
| title: ([_, serviceName]) => `Patient Lists / ${decodeURI(serviceName)}`, |
There was a problem hiding this comment.
Oops, good catch @ibacher I just removed the unused parameter based on the warning without thinking of the ramifications.
| @@ -3,3 +3,51 @@ export const basePath = '/appointments'; | |||
| export const spaBasePath = `${window.spaBase}/home`; | |||
There was a problem hiding this comment.
Future refactoring:
- This should probably be called
spaHomePage - The definition should be
${window.getOpenmrsSpaBase()}home(getOpenmrsSpaBase()is normalised so it always returns a path ending in/.window.spaBaseis a direct representation of whatever was fed into the build the process, and thus may or may not contain a trailing slash).
There was a problem hiding this comment.
Updated, thanks @ibacher !
|
|
||
| export function useTodaysAppointments() { | ||
| const { t } = useTranslation(); | ||
| const { currentAppointmentDate } = useAppointmentDate(); | ||
|
|
||
| const apiUrl = `/ws/rest/v1/appointment/all?forDate=${currentAppointmentDate}`; | ||
| const { data, error, isLoading, isValidating, mutate } = useSWR<{ data: Array<Appointment> }, Error>( | ||
| const { data, error, isLoading, isValidating, mutate } = useSWR<AppointmentsFetchResponse, Error>( | ||
| currentAppointmentDate ? apiUrl : null, | ||
| openmrsFetch, | ||
| ); | ||
|
|
||
| const results = useMemo(() => { |
There was a problem hiding this comment.
minor: we can remove the useMemo as it's just creating an object.
| const appointments = data?.data?.length ? data.data : null; | ||
|
|
||
| const pastAppointments = appointments | ||
| ?.sort((a, b) => (b.startDateTime > a.startDateTime ? 1 : -1)) |
There was a problem hiding this comment.
This is different from the others. is the descending sort intentional?
There was a problem hiding this comment.
I just copied this code over from the patient chart, but will test and review. I could see it making sense that you'd want descending on some views and ascending in others, maybe I need to look closer into that. For instance, in a simple list of a patient's past appointments you might want the most recent in the past first, but for any daily view you probably want the earliest first.
There was a problem hiding this comment.
Yeah, I think the idea is that you want to sort past appointments in a different order (newest first) than upcoming and today (oldest first). Will leave this for now and we can update as user test and review the functionality..
There was a problem hiding this comment.
Thanks @ibacher @chibongho for slogging through this long PR.... I'd added comments, will go ahead and make the changes I mention. I also want to add back in a couple features based on Antony's comments on the ticket. will post when things are ready for re-review.
| @@ -18,14 +18,18 @@ | |||
| "extract-translations": "turbo extract-translations -- --config ../../tools/i18next-parser.config.js" | |||
| }, | |||
| "dependencies": { | |||
| "@hookform/resolvers": "^3.3.1", | |||
There was a problem hiding this comment.
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.
| }, | ||
| "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.
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.
| } | ||
|
|
||
| const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appointment, scheduleType }) => { | ||
| const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appointment, mutate }) => { |
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.
| export const datePickerFormat = 'd/m/Y'; | ||
| export const weekDays = [ | ||
| { | ||
| id: 'MONDAY', |
There was a problem hiding this comment.
this was stuff I just copied directly from the patient-chart when copying the appointment form over, I can look into simplifying it.
| const appointments = data?.data?.length ? data.data : null; | ||
|
|
||
| const pastAppointments = appointments | ||
| ?.sort((a, b) => (b.startDateTime > a.startDateTime ? 1 : -1)) |
There was a problem hiding this comment.
I just copied this code over from the patient chart, but will test and review. I could see it making sense that you'd want descending on some views and ascending in others, maybe I need to look closer into that. For instance, in a simple list of a patient's past appointments you might want the most recent in the past first, but for any daily view you probably want the earliest first.
| const pastAppointments = appointments | ||
| ?.sort((a, b) => (b.startDateTime > a.startDateTime ? 1 : -1)) | ||
| ?.filter(({ status }) => status !== 'Cancelled') | ||
| ?.filter(({ startDateTime }) => | ||
| dayjs(new Date(startDateTime).toISOString()).isBefore(new Date().setHours(0, 0, 0, 0)), | ||
| ); | ||
|
|
||
| const upcomingAppointments = appointments | ||
| ?.sort((a, b) => (a.startDateTime > b.startDateTime ? 1 : -1)) | ||
| ?.filter(({ status }) => status !== 'Cancelled') | ||
| ?.filter(({ startDateTime }) => dayjs(new Date(startDateTime).toISOString()).isAfter(new Date())); | ||
|
|
||
| const todaysAppointments = appointments | ||
| ?.sort((a, b) => (a.startDateTime > b.startDateTime ? 1 : -1)) | ||
| ?.filter(({ status }) => status !== 'Cancelled') | ||
| ?.filter(({ startDateTime }) => dayjs(new Date(startDateTime).toISOString()).isToday()); |
There was a problem hiding this comment.
Will take a look, just copied this code over from patient chart so hesitant to refactor unless I can clearly test.
| export function getAppointmentsByUuid(appointmentUuid: string, abortController: AbortController) { | ||
| return openmrsFetch(`/ws/rest/v1/appointments/${appointmentUuid}`, { | ||
| signal: abortController.signal, | ||
| }); | ||
| } | ||
|
|
||
| export function getAppointmentService(abortController: AbortController, uuid) { | ||
| return openmrsFetch(`/ws/rest/v1/appointmentService?uuid=` + uuid, { | ||
| signal: abortController.signal, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Code I copied over, I'm not sure where they are used as well... I hope to do a more thorough cleanup of all the resource calls in the future, I'll had a TODO to the code to take a look at that then.
Add workload functionality back in
| }; | ||
|
|
||
| export const useCalendarDistribution = ( | ||
| servieUuid: string, |
fix tests rename workload resource filename
add back in all day functionality
changes based on code review
| @@ -18,14 +18,18 @@ | |||
| "extract-translations": "turbo extract-translations -- --config ../../tools/i18next-parser.config.js" | |||
| }, | |||
| "dependencies": { | |||
| "@hookform/resolvers": "^3.3.1", | |||
There was a problem hiding this comment.
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
| }, | ||
| "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.
It's used in the appointment-form.component @ibacher
import { convertTime12to24 } from '@openmrs/esm-patient-common-lib';
| } | ||
|
|
||
| const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appointment, scheduleType }) => { | ||
| const AppointmentActions: React.FC<AppointmentActionsProps> = ({ visits, appointment, mutate }) => { |
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
| export const datePickerFormat = 'd/m/Y'; | ||
| export const weekDays = [ | ||
| { | ||
| id: 'MONDAY', |
There was a problem hiding this comment.
Thinking through this, I'm inclined to keep the id... IMO it's beneficial for each element to have a defined id, even if it just directly maps to the label.
| const appointments = data?.data?.length ? data.data : null; | ||
|
|
||
| const pastAppointments = appointments | ||
| ?.sort((a, b) => (b.startDateTime > a.startDateTime ? 1 : -1)) |
There was a problem hiding this comment.
Yeah, I think the idea is that you want to sort past appointments in a different order (newest first) than upcoming and today (oldest first). Will leave this for now and we can update as user test and review the functionality..
| const pastAppointments = appointments | ||
| ?.sort((a, b) => (b.startDateTime > a.startDateTime ? 1 : -1)) | ||
| ?.filter(({ status }) => status !== 'Cancelled') | ||
| ?.filter(({ startDateTime }) => | ||
| dayjs(new Date(startDateTime).toISOString()).isBefore(new Date().setHours(0, 0, 0, 0)), | ||
| ); | ||
|
|
||
| const upcomingAppointments = appointments | ||
| ?.sort((a, b) => (a.startDateTime > b.startDateTime ? 1 : -1)) | ||
| ?.filter(({ status }) => status !== 'Cancelled') | ||
| ?.filter(({ startDateTime }) => dayjs(new Date(startDateTime).toISOString()).isAfter(new Date())); | ||
|
|
||
| const todaysAppointments = appointments | ||
| ?.sort((a, b) => (a.startDateTime > b.startDateTime ? 1 : -1)) | ||
| ?.filter(({ status }) => status !== 'Cancelled') | ||
| ?.filter(({ startDateTime }) => dayjs(new Date(startDateTime).toISOString()).isToday()); |
There was a problem hiding this comment.
Oh, yes, this makes total sense @ibacher . However :) I'm not sure where this code is currently being used, so out of an abundance of caution I'm not going to refactor something I'm not 100% sure how to test. I suspect this will come over when I refactor the patient appointments to use this, instead of the version in the patient chart.
| @@ -51,7 +51,7 @@ export function startupApp() { | |||
| }, | |||
| { | |||
| path: `${window.spaBase}/patient-list/:forDate/:serviceName`, | |||
| title: ([date, serviceName]) => `Patient Lists / ${decodeURI(serviceName)}`, | |||
| title: ([serviceName]) => `Patient Lists / ${decodeURI(serviceName)}`, | |||
There was a problem hiding this comment.
Oops, good catch @ibacher I just removed the unused parameter based on the warning without thinking of the ramifications.
| @@ -3,3 +3,51 @@ export const basePath = '/appointments'; | |||
| export const spaBasePath = `${window.spaBase}/home`; | |||
There was a problem hiding this comment.
Updated, thanks @ibacher !
fix typo in css
|
whew! I think this is ready to go, going to merge this in, thanks everyone! |
|
Hey, @mogoodrich, @ibacher there's currently a regression involving the Today's appointment widget on the dev3 home page:
From the response, it appears that the Is this likely indicative of an unexpected change in the locations (I'd expect locations to have a Also, @ibacher, a timely reminder that we should come up with a better error state than this:
|
|
thanks @denniskigen ... I missed this this morning, but Mike just pointed it out to me, I will look at next week... |
|
Actually, @denniskigen do you know when this regressed? Was this something that only worked when we were running the custom Palladium build of Bahmni Appointments on dev3 (which we were doing at one point, right). Because a lot of the Bahmni Appointments REST responses are non-standard, and, you are right, it should have a display property, but it doesn't and it looks like it's always been that way: https://github.com/Bahmni/openmrs-module-appointments/blob/master/omod/src/main/java/org/openmrs/module/appointments/web/mapper/AppointmentMapper.java#L266 For now, IMO, we should just build the appointments UI around this non-standard REST response (ie, just use location.name vs location.display)... fyi @mseaton @ibacher |
@mogoodrich / @denniskigen - I have a PR out to address this. The reason for this error is that there is a particular appointment coming back that has a null location. So if null locations are possible on appointments, then the UI just needs to account for that in the display, which it wasn't. On a secondary note, this actually highlights a bug in which the location (if not null) would never have been displayed right due to the use of display over name. I've fixed that in the PR too. |
|
Makes sense, @mseaton ... so I suspect this was never working, period, but wasn't also throwing an error until we had an appointment with a null location on the demo server. |
|
To clarify @denniskigen , @mseaton did point out that it was likely a change I had made that broke this, I had forgotten I had touched that code... |
It looks like this is happening again on dev3 after it got reset @mogoodrich. Is the location a required field when scheduling an appointment?
|
|
Hmmm... I think the issue is that it's not required (at least in the version we're running), but the frontend assumes it's always there. It's likely this is just how the demo data is populated, which should be fixable, |
|
Thanks @denniskigen ! Yes, location should be null-safe, not sure if this was a regression or a place where we never fixed this... I pushed up what should be a quick fix: 3feebb1 I have to step away now but should be able confirm that it fixed it later tonight. |
|
Looks like it is working again, sorry and thanks for pointing it out @denniskigen
|
| padding-top: spacing.$spacing-03; | ||
| } | ||
|
|
||
| :global(.cds--select-input):focus { |
There was a problem hiding this comment.
Hello @mogoodrich is this intended from the designs!, as this affects other fields not on the appointment form.
Thanks
cc @brandones @vasharma05
There was a problem hiding this comment.
It's a very good point @usamaidrsk ... I just copied this scss class over as-is from the patient-chart, but I would think that everything in this class should be namespaces in some way to only to apply to this page... and there's likely stuff that may not be needed at all.
Maybe the best thing to do would be to comment out everything from this class and view the appointments-form from the app and then only add back in things that are necessary for that appointment-form, namespacing as necessary. How does that sound @usamaidrsk ?
There was a problem hiding this comment.
Thanks for the response @mogoodrich , I think we can try with the suggestion you've made.
There was a problem hiding this comment.
Yeah... the two @usamaidrsk flagged are a bit of a weird case.
.formWrapper :global(.cds--select-input):focusWould be safe, but as-written, this is overriding the behaviour of all .cds--select-input elements.
| margin-top: 0.5rem; | ||
| } | ||
|
|
||
| :global(.cds--number input[type='number']):focus { |
There was a problem hiding this comment.
This too @mogoodrich , point mentioned here
|
Screenshots are a requirement in PRs that make UI changes. Let's please enforce that requirement @mogoodrich @ibacher @chibongho . Also not sure why a "refactor" included UI changes at all. A summary would have been helpful. |
|
@brandones summary can be found on the ticket: https://openmrs.atlassian.net/browse/O3-2827 And there were not intended to be design changes, the scss files were just copied from one ESM to another with the assumption that any styling applied in appointments-form.scss would only be applied to the appointments-form, which I was also moving over. However, I looks like I should not have made that assumption, and it wasn't correct... |
https://openmrs.atlassian.net/browse/O3-2827
Requirements
Summary
Screenshots
Related Issue
Other