(feat) O3-4869 (un-revert) Workspace v2 #1459
Merged
Conversation
This file contains hidden or 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
This reverts commit fca7d4a.
chibongho
requested review from
mseaton,
brandones,
mogoodrich,
denniskigen and
ibacher
|
Size Change: -1.48 MB (-18.88%) 👏 Total Size: 6.34 MB
ℹ️ View Unchanged
|
ibacher
approved these changes
Sep 30, 2025
| } | ||
| } | ||
|
|
||
| export function getOpenedWindowIndexByWorkspace(workspaceName: string) { |
There was a problem hiding this comment.
Eventually, we should add TSDoc comments to anything in the public API, which I assume covers all of this.
Comment on lines
+9
to
+14
| export function renderWorkspaceWindowsAndMenu(target: HTMLElement | null) { | ||
| if (target) { | ||
| const root = createRoot(target); | ||
| root.render(<WorkspaceWindowsAndMenu />); | ||
| } | ||
| } |
There was a problem hiding this comment.
I'd also probably prefer if this were done as part of the primary navigation app and not the app shell (since we have some things like the login page which shouldn't need this.
There was a problem hiding this comment.
I wanted to follow the pattern with modals, toasts and notifications to have their root as part of the app shell. Workspace v1 was the odd one out in that we needed to have <WorkspaceContainer> defined in each app.
Comment on lines
117
to
123
| * [¹] Omitting window or group props is useful for workspaces that don't have ties to the window or group "context" (props). | ||
| * For example, in the patient chart, the visit notes / clinical forms / order basket action menu button all share | ||
| * a "group context" of the current visit. However, the "patient list" action menu button does not need to share that group | ||
| * context, so opening that workspace should not need to cause other workspaces / windows / groups to potentially close. | ||
| * The "patient search" workspace in the queues and ward apps is another example. | ||
| * | ||
| * [²] 2 sets of props are compatible if either one is nullish, or if they are shallow equal. |
There was a problem hiding this comment.
It would probably be better to format these as actual Markdown foot notes, i.e., [^1] then [^1]: Blah blah blah. We at least have a chance in correctly creating links for those.
It would be good (eventually) to include one or two @example uses here too.
| * @experimental | ||
| */ | ||
| export async function launchWorkspace2< | ||
| WorkspaceProps extends Record<string, any>, |
There was a problem hiding this comment.
I tend to prefer:
Suggested change
| WorkspaceProps extends Record<string, any>, | |
| WorkspaceProps extends Record<string, unknown>, |
Unless we really can't...
Comment on lines
+299
to
+301
| if (a == null || b == null) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Suggested change
| if (a == null || b == null) { | |
| return true; | |
| } | |
| if (a === null || b === null) { | |
| return true; | |
| } |
Comment on lines
303
to
317
| // check that every prop in a is also in b, and they are equal | ||
| for (let key in a) { | ||
| if (!(key in b) || a[key] !== b[key]) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| // check that every prop in b is also in a, and they are equal | ||
| for (let key in b) { | ||
| if (!(key in a) || a[key] !== b[key]) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
We actually have a shallowEqual() function which should be slightly more efficient.
denniskigen
reviewed
Oct 1, 2025
| <div class="omrs-snackbars-container"></div> | ||
| <div class="omrs-modals-container"></div> | ||
| <div id="omrs-left-nav-container"></div> | ||
| <div id="omrs-workspace-container"></div> |
There was a problem hiding this comment.
Suggested change
| <div id="omrs-workspace-container"></div> | |
| <div id="omrs-workspaces-container"></div> |
To accurately reflect that it renders multiple workspaces instead of a single one.
| grid-area: leftNav; | ||
| } | ||
|
|
||
| #omrs-workspace-container { |
There was a problem hiding this comment.
Suggested change
| #omrs-workspace-container { | |
| #omrs-workspaces-container { |
| } | ||
|
|
||
| function showWorkspacesAndActionMenu() { | ||
| renderWorkspaceWindowsAndMenu(document.querySelector('#omrs-workspace-container')); |
There was a problem hiding this comment.
Suggested change
| renderWorkspaceWindowsAndMenu(document.querySelector('#omrs-workspace-container')); | |
| renderWorkspaceWindowsAndMenu(document.querySelector('#omrs-workspaces-container')); |
| tagContent?: React.ReactNode; | ||
| } | ||
|
|
||
| function Tags({ getIcon, isWindowHidden: isWindowHidden, tagContent }: TagsProps) { |
There was a problem hiding this comment.
Suggested change
| function Tags({ getIcon, isWindowHidden: isWindowHidden, tagContent }: TagsProps) { | |
| function Tags({ getIcon, isWindowHidden, tagContent }: TagsProps) { |
This destructuring alias shouldn't be necessary as it's using the same name as the original.
Comment on lines
+83
to
+97
| const isFocused = isCurrentWindowFocused(openedWindows, window); | ||
|
|
||
| const onClick = async () => { | ||
| if (isWindowOpened) { | ||
| if (!isFocused) { | ||
| restoreWindow(window.windowName); | ||
| } | ||
| } else { | ||
| const shouldLaunch = await (onBeforeWorkspaceLaunch?.() ?? true); | ||
| if (shouldLaunch) { | ||
| const { workspaceName, workspaceProps, windowProps } = workspaceToLaunch; | ||
| launchWorkspace2(workspaceName, workspaceProps, windowProps); | ||
| } | ||
| } | ||
| }; |
There was a problem hiding this comment.
Suggested change
| const isFocused = isCurrentWindowFocused(openedWindows, window); | |
| const onClick = async () => { | |
| if (isWindowOpened) { | |
| if (!isFocused) { | |
| restoreWindow(window.windowName); | |
| } | |
| } else { | |
| const shouldLaunch = await (onBeforeWorkspaceLaunch?.() ?? true); | |
| if (shouldLaunch) { | |
| const { workspaceName, workspaceProps, windowProps } = workspaceToLaunch; | |
| launchWorkspace2(workspaceName, workspaceProps, windowProps); | |
| } | |
| } | |
| }; | |
| const isFocused = useMemo(() => isCurrentWindowFocused(openedWindows, window), [openedWindows, window]); | |
| const onClick = async () => { | |
| try { | |
| if (isWindowOpened) { | |
| if (!isFocused) { | |
| restoreWindow(window.windowName); | |
| } | |
| return; | |
| } | |
| const shouldLaunch = await (onBeforeWorkspaceLaunch?.() ?? true); | |
| if (shouldLaunch) { | |
| const { workspaceName, workspaceProps, windowProps } = workspaceToLaunch; | |
| launchWorkspace2(workspaceName, workspaceProps, windowProps); | |
| } | |
| } catch (error) { | |
| console.error('Failed to handle workspace action:', error); | |
| } | |
| }; |
Could potentially be refactored to this to simplify the complex conditional logic and optimize the computation (less important here).
| // most recently opened action appended to the end | ||
| { | ||
| windowName: windowName, | ||
| openedWorkspaces: [newOpenedWorkspace(workspaceName, workspaceProps)], // root workspace at index 0 |
There was a problem hiding this comment.
Suggested change
| openedWorkspaces: [newOpenedWorkspace(workspaceName, workspaceProps)], // root workspace at index 0 | |
| openedWorkspaces: [newOpenedWorkspace(workspaceName, workspaceProps)], // single workspace in new array |
| return true; | ||
| } | ||
|
|
||
| // check that every prop in a is also in b, and they are equal |
There was a problem hiding this comment.
Suggested change
| // check that every prop in a is also in b, and they are equal | |
| // check that every prop in a is also in b, and they are equal (bidirectional check) |
| const openedWindowIndex = getOpenedWindowIndexByWorkspace(workspaceName); | ||
| const isWindowAlreadyOpened = openedWindowIndex >= 0; | ||
|
|
||
| // if current opened group is not the same as the requested group, or if the group props are different, then prompt for unsaved changes |
There was a problem hiding this comment.
Suggested change
| // if current opened group is not the same as the requested group, or if the group props are different, then prompt for unsaved changes | |
| // Check if we need to prompt for group change due to incompatible props | |
| // if current opened group is not the same as the requested group, or if the group props are different, then prompt for unsaved changes |
| } else { | ||
| return false; | ||
| } | ||
| } else if (isWindowAlreadyOpened) { |
There was a problem hiding this comment.
Suggested change
| } else if (isWindowAlreadyOpened) { | |
| // Handle case where window is already opened | |
| } else if (isWindowAlreadyOpened) { |
| return false; | ||
| } | ||
| } | ||
| } else { |
There was a problem hiding this comment.
Suggested change
| } else { | |
| // Create new window when none exists | |
| } else { |
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.
This reverts commit fca7d4a.
Requirements
feat,fix, orchore, among others). See existing PR titles for inspiration.If applicable
Summary
This PR un-reverts the initial workspace v2 check-in (#1424), which was reverted in fca7d4a for the 3.5 release.
This should not affect how the current workspace system works.
Screenshots
Related Issue
Other