-
Notifications
You must be signed in to change notification settings - Fork 59
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
(feat) O3-2006 Adds ability to launch workspace from form #186
Conversation
Size Change: -480 kB (-28%) 🎉 Total Size: 1.22 MB
ℹ️ View Unchanged
|
src/components/inputs/workspace-launcher/workspace-launcher.tsx
Outdated
Show resolved
Hide resolved
@pirupius FYI, the attached demo seems to be powered by the AFE and not RFE. |
Thanks @samuelmale will correct that video. |
66cf176
to
8a15286
Compare
8a15286
to
b83c910
Compare
@samuelmale the video has been updated to use |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very close, just a few minor comments
); | ||
}; | ||
|
||
export default WorkspaceLauncher; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pirupius can you suffix the component file name with ".component.tsx"
import React from 'react'; | ||
import { useTranslation } from 'react-i18next'; | ||
import { showSnackbar } from '@openmrs/esm-framework'; | ||
import { useLaunchWorkspaceRequiringVisit } from '@openmrs/esm-patient-common-lib/src/useLaunchWorkspaceRequiringVisit'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For consistency:
import { useLaunchWorkspaceRequiringVisit } from '@openmrs/esm-patient-common-lib/src/useLaunchWorkspaceRequiringVisit'; | |
import { useLaunchWorkspaceRequiringVisit } from '@openmrs/esm-patient-common-lib'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the recommended way in cases where we need imports from a single source rather than the whole module for faster optimization. Similar usages have been used in other packages within 3.x for example in the medications HERE
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the recommended way in cases where we need imports from a single source rather than the whole module for faster optimization.
I understand the idea @pirupius but I would stick to the standard imports until there is a clear consensus around this. Maybe in the future we gotta document this to make it well known that it's the recommended way. We may have to do the same for framework imports for more consistency and also the optimisation bit.
@@ -1488,6 +1488,41 @@ __metadata: | |||
languageName: node | |||
linkType: hard | |||
|
|||
"@carbon/react@npm:^1.12.0": | |||
version: 1.55.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you intend to do a Carbon bump?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was a side effect of the patient commons lib being added. It requires ^1.55.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The commons lib requires v^1.12.0 but currently hydrated with v1.40.0
b83c910
to
379200c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM thanks @pirupius 🍻
Requirements
Summary
Adds support to launch the workspace within a form with the addition of
workspace-launcher
Screenshots
Form builder
Forms workspace within patient chart
Screen.Recording.2024-04-22.at.12.04.04.mov
Related Issue
Other