Skip to content
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

(fix) O3-2426: Closing drug search panel should send user back to order basket if they previously had it open #1452

Closed
wants to merge 7 commits into from

Conversation

jwnasambu
Copy link
Contributor

@jwnasambu jwnasambu commented Nov 3, 2023

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

I added openDrugSearch function which Launches a patient workspace with the name 'add-drug-order' and passes an object with a property onCloseWorkspace, which is set to the onClose function so that when the 'add-drug-order' workspace is launched, the onClose function closes a workspace with the name 'add-drug-order' and passes true as a parameter. It then launches a patient workspace with the name 'order-basket'.

Screenshots

screencast.2023-11-07.12.AM-54-03.mp4

Related Issue

https://issues.openmrs.org/browse/O3-2426

Other

@jwnasambu jwnasambu marked this pull request as draft November 3, 2023 13:16
@jwnasambu jwnasambu marked this pull request as ready for review November 7, 2023 11:49
Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So this looks correct to me for what’s specified in the ticket. I do think that, eventually, we’ll need to add some more complex state (specifically, if the user just opens the drug order panel, they should get the close button and the close button should restore the previous workspace and a better solution would introduce some sort of “workspace stack”), but this is an improvement on the current behaviour.

It would be nice to also integrate the icon change @vasharma05 suggested in the ticket.

Thanks @jwnasambu for this!

@vasharma05
Copy link
Member

Hi @ibacher !
Thanks for the review, but I've a small modification in this to do before merging this. I'll push the change.

Instead of either choosing the onClose /closeWorkspace, the closeWorkspace should call the onClose function.

@ibacher ibacher changed the title (fix)03-2426: Closing drug search panel should send user back to order basket if they previously had it open (fix) O3-2426: Closing drug search panel should send user back to order basket if they previously had it open Nov 21, 2023
@brandones
Copy link
Contributor

brandones commented Nov 25, 2023

The workspace system already operates like a stack, but the trouble is that it only supports one workspace of each type. Since the order-basket and add-drug-order workspaces have the same type, only one can exist in the workspace stack at a time, so when add-drug-order opens it displaces order-basket.

Let's weigh the two approaches:

onClose

This is preferable if we always want one workspace to close into another, regardless of where the user came from. So if we want sequences like

  • (A) active medications widget --> (click add drug) --> add-drug-order workspace --> (click close) --> order basket workspace
  • (B) order basket workspace --> (click add drug) --> add-drug-order workspace --> (click close) --> order basket workspace

which, the latter I'm sure about, the first I don't know. I have forgotten the context in which I made the ticket. But the ticket suggests that we want just the latter, not the first.

stack

This is preferable if we want to go to the last workspace only if it was previously open. This is already how it works for workspaces of different types. This is if we want the sequences

  • (C) active medications widget --> (click add drug) --> add-drug-order workspace --> (click close) --> workspace window closes
  • (B) order basket workspace --> (click add drug) --> add-drug-order workspace --> (click close) --> order basket workspace

If it's the case that we actually want flow (A), then we really want to have the onClose implementation instead, or else we will be doing funny things where we make all calls to launchPatientWorkspace('add-drug-order') also do launchPatientWorkspace('order-basket') right before, which sounds fragile and annoying to maintain.

I'm not sure if this will apply to any other workspace flows (e.g. the form workspace; if a user closes a form, should it take them back to the form selector?).

If we want to support multiple workspaces of the same type in the stack, maybe we can just add a parameter to launchPatientWorkspace which makes it do that. Which sounds easy enough, so I suggest we figure this out and just do it right the first time.

@ibacher
Copy link
Member

ibacher commented Nov 27, 2023

So, I think going to the last workspace only if it was previously opened makes the most sense. Specifically, I think that if the user opens a workspace from the side rail and then opens a child workspace (form entry / add drug order / add lab order / etc.), when closing the child workspace it should go back to the parent. Conversely, if a "child workspace" (form entry / add drug order / add lab order / etc.) is opened directly from a patient chart component, (e.g., the "Add +" buttons, Actions menus), it seems appropriate to simply close the workspace once that "child workspace" is done.

There are some other cases that I think also fit with this pattern, specifically, launching what I've aboved termed "child workspace"s from a workspace that is not normally the workspaces parent. For example, if in a form I want to add a Condition, it would be appropriate to open the "add condition" workspace, but when that workspace is closed, I should be returned to the form, even though the Condition workspace is not normally part of the form side-rail app. Similarly, with the task list, if I have a task to order a medication for a patient, I should be able to click the link, view the add-drug-order workspace and, when I'm done, still be back on the task list.

It's hard for me to conceive of a case where the "right" behaviour is always to go back to some default-per-workspace page (e.g., I opened the add-drug-order workspace from the task list, when I'm done with the order, I probably want to be looking at the task list, not the order basket).

There are some clear corner cases implied by this, though. For example, if I open the "Add Conditions" workspace from the condition list view, then the workspace should probably open in what the UX docs call the "Independent actions" variant, i.e., with a close button instead of a collapse button. But if I open the "Add Conditions" workspace from within a form (which has a side-rail icon), it should be opened in what the UX docs call the "Siderail" variant, with a collapse button that collapses back to the form entry side rail. (This also means that if I've opened the "Add Conditions" workspace from within a form and collapsed it, when I re-open the form workspace from the side rail, I should see the "Add Conditions" page in the state that I left it in).

@brandones
Copy link
Contributor

Ok, sounds like let's go ahead with making the workspace stack tolerate multiple workspaces of the same type (when the right param is passed to launchPatientWorkspace), @vasharma05 . Sorry for the changing requirements.

@brandones
Copy link
Contributor

Thanks for your work on this @jwnasambu , sorry about the change in approach.

@brandones brandones closed this Jun 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants