(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.
|
@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.
There was a problem hiding this comment.
Ok. Can we update the docs to match the code?
| 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.
There was a problem hiding this comment.
The only reason I bring this up is because of how it gets compiled. Specifically, since the optional operator isn't supported across browsers, e.g.,
store?.getState()?.['option']Gets can get compiled into this gnarly mess:
var _store_getState, _store;
(_store = store) === null || _store === void 0 ? void 0 : (_store_getState = _store.getState()) === null || _store_getState === void 0 ? void 0 : _store_getState['option'];Which is semantically the same, but which involves 3 branching statements and 2 comparisons per branch ("native" support for ?, this is pushed into the interpreter itself, which is usually faster).
| }); | ||
| 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.
| * 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.
There was a problem hiding this comment.
Are clients expected to call both of these functions? When would I, as a code consumer, call launchWorkspaceGroup() instead of launchWorkspace()?
There was a problem hiding this comment.
No, to launch the workspace group, the client only needs to call the launchWorkspaceGroup function and they can pass the new workspace's details into the argument. You can refer to the argument structure here.
Thanks!
|
Hi @ibacher! |
|
Hi @ibacher! |
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