( fix ) - O3-3171 Replace overlays in the Appointments app #1133
Conversation
kb019
force-pushed
the
fix/O3-3171
branch
2 times, most recently
from
8685460
to
26ec9c4
Compare
...ges/esm-appointments-app/src/appointments/common-components/appointments-table.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-appointments-app/src/metrics/metrics-header.component.tsx
Outdated
Show resolved
Hide resolved
|
Please get rid of the explicit passing of You can probably delete At some point we should probably refactor the appointments system. It is weird that it has three different form names for the same component and that there is a |
|
Hey @brandones 😃. Thank you so much for reviewing. I have made the changes suggested by you. Also a overlay is used by a widget in appointments app which is rendered in extension slot of home app . The Component is here . So should i replace that by workspaceOverlay or keep as it is . |
|
|
||
| export const createAppointment = getSyncLifecycle(appointementsForm, options); | ||
|
|
||
| export const addAppointment = getSyncLifecycle(appointementsForm, options); |
There was a problem hiding this comment.
Defer to @brandones but wouldn't it make sense to the just create a single "appointmentForm" here instead of "editAppointmentForm", "createAppointment" and "addAppointment" and then just that same component when registering the three different workspaces below?
There was a problem hiding this comment.
sure @mogoodrich , got your point . Will work on that and update the progress . Thank you .
There was a problem hiding this comment.
Yeah that would be great, thanks @mogoodrich , thanks @kb019
We want to migrate everything to the new workspace system. There should only be one workspace renderer per page, and the home page already has one, so we probably don't need that overlay component there at all. Please make sure to find where it is in the UI and verify that everything still works after you make the change. |
kb019
force-pushed
the
fix/O3-3171
branch
3 times, most recently
from
44e77bc
to
2ba5221
Compare
|
Hi @brandones and @mogoodrich , I have made the necessary changes . Could you please review it. I have attached the updated video.Thanks @mogoodrich .Thanks @brandones . |
There was a problem hiding this comment.
LGTM, assuming all the overlays have been tested and still open, thanks @kb019 !
I also just triggered a run of the tests to confirm they pass.
Thanks @mogoodrich. |
No need to assume, that's what the video is for 😆 Thanks @kb019 , and thanks for the review @mogoodrich ! |
|
So there's definitely a bug with this PR, see attached screencast of I took of toggling the selectors on the appointments form before and after the change. Before: Screencast.from.05-28-2024.10.12.29.AM.webmAfter: Screencast.from.05-28-2024.10.14.23.AM.webmCan we please fix @kb019 , fyi @brandones ? Also, I don't know if the overlay size is suppose to change as it did (it's wider now). |
Thank you @mogoodrich for the info.I will work on it and update the progress. |
|
Hi @mogoodrich and @brandones , sorry for not checking this out earlier. I have raised a pr for this here could you please look into it. |
Co-authored-by: Mark Goodrich <mgoodrich@pih.org>
Requirements
Summary
To replace the overlay in the esm-appointments-app with the workspaces. Also i think the size of the workspaceOverlay seems quite big which comes from the min-width property assigned in the esm-framework.
https://openmrs.atlassian.net/browse/O3-3171
Video
OpenMRS.-.Personal.-.Microsoft_.Edge.2024-05-09.12-45-54.webm