(BREAKING) O3-4077: Improve the workspace group workflow #1185
Conversation
vasharma05
requested review from
mogoodrich,
ibacher,
chibongho,
denniskigen,
NethmiRodrigo and
usamaidrsk
|
Size Change: -148 kB (-2.36%) Total Size: 6.14 MB
ℹ️ View Unchanged
|
|
With this change, does it mean that the Also, is there a way to close a all workspaces in a workspace family? If yes to the above questions, then yeah, I'm generally on board. This change is a bit hard for me to review without seeing it in action. If people are on board with this, maybe we can also have a draft for corresponding changes in patient chart or patient management? |
Exactly, we won't have to duplicate workspaces to configure it and to open it in the workspace family, now we use the same workspace either individually or in the workspace family, with minimal to no changes. I am using the same to add Order Basket to the Ward Workspace.
Yes, we have a function to close all workspaces, I need to tweak it to allow closing only the workspaces in a family.
I understand, hence I am implementing this with the order basket addition to the ward workspaces, and will share the results here, currently keeping this PR in draft and will make changes as I find more observations. Thanks @chibongho for supporting the work! |
vasharma05
force-pushed
the
refactor/O3-4077
branch
from
6d38885
to
49b6241
Compare
packages/framework/esm-styleguide/src/workspaces/container/workspace-container.component.tsx
Outdated
Show resolved
Hide resolved
packages/framework/esm-styleguide/src/workspaces/container/workspace-container.component.tsx
Outdated
Show resolved
Hide resolved
...s/framework/esm-styleguide/src/workspaces/workspace-sidebar-store/useWorkspaceFamilyStore.ts
Outdated
Show resolved
Hide resolved
|
@brandones @ibacher @denniskigen, this PR is open for review now. Hoping for your timely review. |
|
As a total aside: looking at this code I think we should wrap the patient chart state in a higher-level API. Something like |
packages/framework/esm-framework/docs/interfaces/CloseWorkspaceOptions.md
Outdated
Show resolved
Hide resolved
packages/framework/esm-styleguide/src/workspaces/container/workspace-container.component.tsx
Show resolved
Hide resolved
packages/framework/esm-styleguide/src/workspaces/workspaces.test.ts
Outdated
Show resolved
Hide resolved
|
I've provided some feedback. I did some pairing with Vineet and Usama on this so it would be great if someone else could review after the next revision. Generally I think the approach is good. @mogoodrich @chibongho @denniskigen @ibacher |
vasharma05
changed the title
There was a problem hiding this comment.
Thanks. I think this will simplify the workspace sidebar
Questions:
- Should a workspace instance have access to its
currentWorkspaceGroup(since it is required to close its workspace group)? - When
launchWorkspaceis called from within a workspace or from an action menu item, does the new workspace get launched within the same workspace group? - Is it still true that a
WorkspaceContaineris not tied to any particular workspace or workspace group? (so we don't need to create multiple WorkspaceContainer for multiple workspace groups?) - Tangent. I'm not that comfortable with the idea of
contextKeyforWorkspaceContainer, which defines the URL subpath to match on to determine whether any workspaces in theWorkspaceContainershould remain open. It feels like we should just close any open workspaces when the<WorkspaceContainer>dismounts. Thoughts?
| @@ -171,7 +183,7 @@ function Workspace({ workspaceInstance, additionalWorkspaceProps }: WorkspacePro | |||
| <div className={styles.overlayHeaderSpacer} /> | |||
| <HeaderGlobalBar className={styles.headerButtons}> | |||
| <ExtensionSlot | |||
| name={`workspace-header-family-${workspaceInstance.sidebarFamily}-slot`} | |||
| name={`workspace-header-family-${workspaceInstance.currentWorkspaceGroup}-slot`} | |||
There was a problem hiding this comment.
This will break things, but should we also rename this to not say "family"?
There was a problem hiding this comment.
No @chibongho, this will not break anything, because the previous sidebarFamily is the same as the currentWorkspaceGroup for the ward app.
| */ | ||
| sidebarFamily?: string; | ||
| preferredWindowSize?: WorkspaceWindowState; | ||
| groups: Array<string>; |
There was a problem hiding this comment.
Do we need this? Shouldn't what group a workspace belongs to be defined by the caller of launchWorkspaceGroup?
There was a problem hiding this comment.
No, the registration of the workspace is the right place to define the groups, else, the developer will have to handle different scenarios of whether to open it in the group or individual, which right now is handled by the workspace system.
There was a problem hiding this comment.
Ok, I think I misunderstood our earlier conversation. Does that mean that a workspace must have a workspace group defined in routes.json for it to be launched in that workspace group? (in other words, it's impossible to create a workspace that can be launched in any arbitrary workspace group.)
I'm thinking of something like the patient search workspace in esm-patient-serach-app, where it's really meant for other apps (ex: esm-service-queues-app) to launch it. There is no way to launch the patient search workspace in the "service-queues-app" group, correct? (Admittedly, if we really want this, we can work around it by having the patient search workspace be an extension instead, and slot it into a workspace in the "service-service-app" group defined within esm-service-queues-app)
There was a problem hiding this comment.
Ok, I think I misunderstood our earlier conversation. Does that mean that a workspace must have a workspace group defined in routes.json for it to be launched in that workspace group? (in other words, it's impossible to create a workspace that can be launched in any arbitrary workspace group.)
Yes
There was a problem hiding this comment.
Admittedly, if we really want this, we can work around it by having the patient search workspace be an extension instead, and slot it into a workspace in the "service-service-app" group defined within esm-service-queues-app
I didn't understand this
There was a problem hiding this comment.
Sorry for the typos. I was saying if we need the patient search workspace defined in patient-search-app to belong to either the "service-queues-app" or "ward-app" workspace group (depending where we want to open it from), then the patient search workspace probably shouldn't be a workspace at all, but instead an extension that can be slotted into a workspace.
There was a problem hiding this comment.
Or just a set of React components, so it can be added to the styleguide? That seems the cleanest to me...
| @@ -440,29 +441,29 @@ describe('workspace system', () => { | |||
| expect(store.getState().openWorkspaces.length).toBe(0); | |||
| }); | |||
|
|
|||
| describe('Testing `getWorkspaceFamilyStore` function', () => { | |||
| describe('Testing `getWorkspaceGroupStore` function', () => { | |||
There was a problem hiding this comment.
nit: replace "Family" in variables names inside this function
|
@chibongho thanks for the thoughtful review, much appreciated. Just a note:
The reason we designed it this way is because it doesn't always line up well with mounts/unmounts. In particular, when transitioning between different patients in the patient chart, the WorkspaceContainer will not be unmounted; however, any open workspaces should certainly be closed. On the home page, I don't remember whether things like Service Queues are dashboards or independent pages that just happen to be served under the |
vasharma05
changed the title
Yes, when the
If the workspace registration (inside the
Yes, we just need 1 workspace container |
|
Hi @brandones @chibongho , |
Shouldn't the group name also be accessible in
Ok, that makes sense. |
What's the use case for this? As far as the common data of a group is concerned, all the common data is shared in the workspace props, as previously implemented. |
Ok, never mind. Your answer to the other question makes this moot. I was thinking something like this: But since |
ibacher
changed the title
There was a problem hiding this comment.
Great overall work here, aside from the nit-picking comments I left.
There's some documentation work to be improved and, since this affects the routes.json file, we should also have a a PR to update the schema.
| @@ -12,18 +11,18 @@ import type { StoreApi } from 'zustand/vanilla'; | |||
| * | |||
| * @param {string} sidebarFamilyName The sidebarFamilyName of the workspace used when registering the workspace in the module's routes.json file. | |||
| */ | |||
| export function useWorkspaceFamilyStore(sidebarFamilyName: string) { | |||
| export function useWorkspaceGroupStore(workspaceGroupName?: string) { | |||
There was a problem hiding this comment.
Why is workspaceGroupName optional here?
There was a problem hiding this comment.
Since this function was initially made to be called for workspaces launched with and without a group name, the argument is hence optional. If the argument is undefined, this function will return undefined, else will return a global store.
I have already added tests for this function.
| expect(sidebarFamilyStore?.getState()?.['foo']).toBe(true); | ||
| launchWorkspace('transfer-patient-workspace', { bar: false }); | ||
| expect(workspaceGroupStore).toBeTruthy(); | ||
| expect(workspaceGroupStore?.getState()?.['foo']).toBe(true); |
There was a problem hiding this comment.
Just a comment, but since we've already asserted that workspaceGroupStore is truthy, we can assume below that it's not null or undefined.
There was a problem hiding this comment.
It has become a habit of mine to use null checks, so that I can check things validity, and also it doesn't break the code at any point. I'll try to improve on this.
| */ | ||
| sidebarFamily?: string; | ||
| preferredWindowSize?: WorkspaceWindowState; | ||
| groups: Array<string>; |
There was a problem hiding this comment.
Or just a set of React components, so it can be added to the styleguide? That seems the cleanest to me...
| * - Updates the main workspace store to remove the workspace group | ||
| * - Calls the optional closeup callback if provided | ||
| */ | ||
| function closeWorkspaceGroup(groupName: string, onWorkspaceCloseup?: Function) { |
There was a problem hiding this comment.
I always prefer () => void over Function. It's substantially more explicit.
| workspaceGroup: undefined, | ||
| })); | ||
| if (onWorkspaceCloseup && typeof onWorkspaceCloseup === 'function') { | ||
| onWorkspaceCloseup?.(); |
There was a problem hiding this comment.
We already did the checking that this is defined, so...
| onWorkspaceCloseup?.(); | |
| onWorkspaceCloseup(); |
| }); | ||
| return; | ||
| } | ||
| const currentGroupName = store.getState().workspaceGroup?.name; |
There was a problem hiding this comment.
| const currentGroupName = store.getState().workspaceGroup?.name; | |
| const currentGroupName = currentWorkspaceGroup?.name; |
There was a problem hiding this comment.
After closing the workspace group, the updated workspace group might have been updated, hence I am fetching the workspace group name from the state.
| .every(({ name }) => { | ||
| const canCloseWorkspace = canCloseWorkspaceWithoutPrompting(name); | ||
| return canCloseWorkspace; | ||
| }); |
There was a problem hiding this comment.
| .every(({ name }) => { | |
| const canCloseWorkspace = canCloseWorkspaceWithoutPrompting(name); | |
| return canCloseWorkspace; | |
| }); | |
| .every(({ name }) => canCloseWorkspaceWithoutPrompting(name)); |
| @@ -302,19 +302,15 @@ export type WorkspaceDefinition = { | |||
| * The width "extra-wide" is for workspaces that contain their own sidebar. | |||
| */ | |||
| width?: 'narrow' | 'wider' | 'extra-wide'; | |||
| preferredWindowSize?: WorkspaceWindowState; | |||
| * Workspaces can open either individually or in a group of workspaces. The workspace groups | ||
| * will define the groups in which a workspace can be opened. | ||
| * | ||
| * In case the currently opened workspace is not present in the groups of a workspace, | ||
| * the current group will close and then the workspace will be launched. |
There was a problem hiding this comment.
I feel like the description here is unclear. Maybe:
| * Workspaces can open either individually or in a group of workspaces. The workspace groups | |
| * will define the groups in which a workspace can be opened. | |
| * | |
| * In case the currently opened workspace is not present in the groups of a workspace, | |
| * the current group will close and then the workspace will be launched. | |
| * Workspaces can open either independently or as part of a "workspace group". A | |
| * "workspace group" groups related workspaces together, so that only one is visible | |
| * at a time. For example, <USE CASE DESCRIPTION HERE>. | |
| * | |
| * In case the currently opened workspace is not present in the groups of a workspace, | |
| * the current group will close and then the workspace will be launched. |
| * In case the currently opened workspace is not present in the groups of a workspace, | ||
| * the current group will close and then the workspace will be launched. |
There was a problem hiding this comment.
These last two lines are really ambiguous. Can you think of a way to rephrase it? I'm not entirely sure what this is trying to communicate, so I'm not sure what to suggest.
There was a problem hiding this comment.
What's unclear to me: What does "currently opened workspace" here mean? Is the workspace being opened, the workspace currently opened, any workspace in the workspace stack? What does this have to do with "launching"?
| * workspaceGroupCleanup: () => console.log("Cleaning up workspace group") | ||
| * }); | ||
| */ | ||
| export function launchWorkspaceGroup(groupName: string, args: LaunchWorkspaceGroupArg) { |
There was a problem hiding this comment.
If a workspace can only ever be opened within the workspace group declared in routes.json, do we need a separate launchWorkspaceGroup function instead of just using launchWorkspace (and overload it it with group related params)?
If we still want to keep this function, we might want to make it generic on both workspace props and workspace group props.
There was a problem hiding this comment.
Yes, it makes sense to have a separate launchWorkspaceGroup function for the following reasons:
- Making clear distinction between launching workspace and launching workspace group. Providing different function orderloads will in the end confuse the developer too.
launchWorkspacefunction is used for every workspace launch, and on the other hand,launchWorkspaceGroupfunction is used only at the initial launch.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.For changes to apps
If applicable
Summary
Currently, for the related workspaces (current implementation: patient dashboard workspace in the Ward View), the sidebar family (group of workspaces is termed as a family) is defined in the workspace registration using 2 properties
hasSidebarandsidebarFamily.@brandones suggests that we can move this handling to the
WorkspaceContainer, and handling of workspace group should be independent to the workspace implementation.New implementation
Workspace group (rename of the workspace family)
Workspace group refers to the workspaces with a sidebar and shares some common state among different workspaces.
Launching workspace group
Workspace group should be launched with
launchWorkspaceGroupand we can pass state, as well as a callback when workspace is launched and a cleanup function when the workspace group closes.Supported group for a workspace
A new property
workspaceGroupsis defined in the workspace registration, to validate if a workspace launched is a part of the currently opened workspace group and if not, the current workspace group will be closed and then the workspace will be launched.Example
Screenshots
None
Related Issue
https://issues.openmrs.org/browse/O3-4077
Other