(feat) Add 'ward-patient' group to the order basket related workspaces #2130
+16
−3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Hi @brandones @ibacher @chibongho, |
vasharma05
force-pushed
the
feat/add-order-basket-to-ward
branch
from
b019a98 to
603732f
Compare
|
Size Change: +30 B (0%) Total Size: 16.1 MB ℹ️ View Unchanged
|
|
Any reason why the build job is failing during the Thanks! |
No So, solution-wise, I don't think what we're doing in this PR is sustainable. We need a way of the ward app to be able to declare that these are the workspaces that it's using. This suggests to me that we need to reconsider being able to set the group during the call to launch workspace or some other mechanism that doesn't require modifying the workspace registration. |
If we pass the group name in the |
vasharma05
force-pushed
the
feat/add-order-basket-to-ward
branch
from
603732f to
1260a00
Compare
|
Well, passing it to It may be that we need to modify the |
We don't have to, because as per the design and confirmed with Paul, only one workspace group can be opened at a time. Also, the
Yes, this was also suggested by @usamaidrsk. However, the drawback is that whenever I want to add a workspace to a workspace group, I have to go to the workspace group's registration and update it. But why does the workspace group need to handle this scenario? In-case someone builds on top of the ward app, they will have to come to the ref app and define their workspace in the workspace group's registration. My question is why does the workspace group need to know about all the workspaces defined for that group, and if it wants the information, the opened workspace stores the currentWorkspaceGroup info, so that is also convenient. I hope I answered your questions |
|
Right now, I have a scenario where I want to include the patient search workspace from patient-search-app in a new workspace group I think this is contrary to the separation of concern design pattern that extensions / workspaces system otherwise gives us.
|
|
Right @vasharma05 at the end of the day I think this PR, as @chibongho points out, demonstrates a violation of our design principles. So we have to tweak the workspace groups system so that we can have a proper separation of concerns. The patient chart should not need to know about the existence of the ward app for any reason. |
|
Yes, @chibongho, you need to add the @brandones @ibacher, let me work on defining workspace groups more explicitly as you have discussed. |
|
Hi @ibacher @brandones, |
vasharma05
force-pushed
the
feat/add-order-basket-to-ward
branch
from
366c5d1 to
005e735
Compare
|
@vasharma05 the build and e2e tests are failing |
|
Sorry I didn't see this til the new year, happy new year @vasharma05 :) I would much rather we fix the workspace group system and get this right, than keep building on something we know we're going to have to breaking-change later. |
|
This is a change we'll need for a project with quite a tight timeline, in other words, we wanted this to be part of If we decide to merge this as is (which of course is far from ideal), can this be considered to be added as a patch of |
We're currently doing an RC of 3.2.0, just for clarity.
Yes, but the problem is that this PR kind of demonstrates that the prior PR (which is included in the release candidate) isn't really fit for purpose. Timeline-wise, we have—I think—at least a week before the next planned RC or release. Is it possible to get a "proper" implementation done in that time-frame? If it's not, we can probably discuss a way forward. |
|
Hi @ibacher @brandones! Wishing you both a happy new year 🎉 As per suggestions in the texts above, I have made changes openmrs/openmrs-esm-core#1256 and openmrs/openmrs-esm-patient-management#1428, which will now allow the workspace group's registration to define the workspaces which are |
vasharma05
force-pushed
the
feat/add-order-basket-to-ward
branch
from
99f716c to
80c1d24
Compare
ibacher
requested changes
Jan 8, 2025
| @@ -35,6 +35,11 @@ | |||
| "fullWidth": false | |||
| } | |||
| }, | |||
| { | |||
There was a problem hiding this comment.
This is another thing I'm going to object to, since we already have an extension for this component. For now, I guess we can just change slot to slots in the existing definition, but we need to come up with something better long-term.
There was a problem hiding this comment.
Yes, ideally we should, but the original definition has an order=0 which isn't the case with the ward-patient action menu.
| }); | ||
| }; | ||
|
|
||
| const openDrugForm = (order: DrugOrderBasketItem) => { | ||
| closeWorkspace('order-basket', { | ||
| ignoreChanges: true, | ||
| onWorkspaceClose: () => launchPatientWorkspace('add-drug-order', { order }), | ||
| closeWorkspaceGroup: false, |
There was a problem hiding this comment.
Shouldn't this be the default, assuming that there's at least one workspace in this group that hasn't been closed?
There was a problem hiding this comment.
that there's at least one workspace in this group that hasn't been closed?
Actually no, since the new workspace is launched only after the previous workspace closes. For now, it's not possible to know whether a new workspace will be opened after closing the current one or not, hence we have the closeWorkspaceGroup property set to true by default to clear the workspace group and run cleanup. In case we don't want to clear the workspace group, similar to the above use case, we can pass the closeWorkspaceGroup to false.
Refer: https://github.com/openmrs/openmrs-esm-core/blob/78b7975f08b975e6080c5224bb745bd8be97beba/packages/framework/esm-styleguide/src/workspaces/workspaces.ts#L249
This reverts commit 80c1d24.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Requirements
Summary
This PR adds the necessary changes to add the order basket to the ward app:
ward-patientgroup name to the order basket related workspaces registrationScreenshots
Screen.Recording.2024-12-12.at.17.46.54.mov
Related Issue
None
Other
Depends on openmrs/openmrs-esm-core#1185