(feat) O3-3799: Open HTML Form Entry Forms within Form Workspace #1963
Conversation
This file contains 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
|
Size Change: -819 kB (-5.41%) ✅ Total Size: 14.3 MB
ℹ️ View Unchanged
|
mogoodrich
commented
Aug 15, 2024
There was a problem hiding this comment.
Definitely not ready for review, but I'm looking for initial feedback on the best way to go about this.
Screencast of how this looks:
Screencast.from.08-14-2024.03.35.54.PM.webm
Also link to ticket: https://openmrs.atlassian.net/browse/O3-3799
| navigate({ | ||
| to: `\${openmrsBase}/htmlformentryui/htmlform/${htmlForm.formUiPage}.page?patientId=${patientUuid}&visitId=${visitUuid}&definitionUiResource=${htmlForm.formUiResource}&returnUrl=${window.location.href}`, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Previously, if this form had been configured as an HTML Form, we simply redirected to the O2 app,
| )} | ||
| {showForm && formInfo && patientUuid && patient && !htmlForm && ( | ||
| <ExtensionSlot name="form-widget-slot" state={state} /> | ||
| )} |
There was a problem hiding this comment.
Now, instead of navigating to the O3 app, we render a new HTML Form Entry Wrapper component, which basically just loads the O2 app in an iframe (see below).
The way I've set up here, by just referencing the Html Form Entry Wrapper within form-entry.workspace, is likely wrong, but I wasn't sure what the best approach was. Should I simply render a different extension slot ("html-form-entry-widget-slot") and define a new extension thta uses HtmlFormEntryWrapper? Should there still just be one extension slot ("form-widget-slot") and what gets wired into it varies? Or should we have an entirely different "html-form-entry.workspace". Apologies for my naivety with best practices/uses of workspaces. :)
One issue I'm seeing right now, that may factor into this decision, is that if you shift between the form workspace and another workspace (ie the order workspace) the data you can currently entered into the form, but have not saved, is sometimes lost. Once we get this set up the "right" way, I can troubleshoot this some more and hopefully we can come up with a workable solution.
Also note that we pass in a "returnUrl" of "post-message:close-workspace", more on this below.
There was a problem hiding this comment.
I think the solution to the "losing form data" may be to have a unique workspace for this? Not sure, but I think that's the case.
There was a problem hiding this comment.
Yeah, let me experiment with doing this in the it's own workspace.
| src: string; | ||
| } | ||
|
|
||
| const HtmlFormEntryWrapper: React.FC<HtmlFormEntryWrapperProps> = ({ closeWorkspace, src }) => { |
There was a problem hiding this comment.
So basically all this does is load the O2 app into an iframe and hides the headers and the breadcrumbs
packages/esm-patient-forms-app/src/htmlformentry/html-form-entry-wrapper.component.tsx
Outdated
Show resolved
Hide resolved
| border: none; | ||
| margin: 0; | ||
| padding: 0; | ||
| height: 3000px; // TODO: how to make this dynamic? |
There was a problem hiding this comment.
My limited CSS skills were failing here... :) I basically want the iframe height to be equal to the height of the content it is displaying.
There was a problem hiding this comment.
I actually think this is quite hard! Most DOM elements have some way of determining the height of their contents so you could just use fit-content, but iframes are special, because they can load cross-origin content. The best I can think of is adding to the onLoad function so it becomes something like:
const onLoad = useCallback(() => {
const iframe = iframeRef.current;
const dashboard = iframe.contentDocument;
dashboard.querySelector('header')?.remove();
dashboard.querySelector('.patient-header')?.remove();
dashboard.querySelector('#breadcrumbs')?.remove();
iframe.style.display = 'block';
iframe.height = iframe.contentWindow.document.body.scrollHeight;
}, []);There was a problem hiding this comment.
And this worked too, once I futzed around a bit and added a little padding... I will fiddle with this a little more....
mogoodrich
requested review from
icrc-jofrancisco,
denniskigen,
donaldkibet,
brandones,
ibacher,
mseaton and
chibongho
|
Related HFE-UI PR to support closing the workspace when saving/cancelling: |
mseaton
reviewed
Aug 15, 2024
There was a problem hiding this comment.
Generally looks good to me @mogoodrich - nice! Defer to others on the semantic questions you are asking to get this done.
ibacher
reviewed
Aug 15, 2024
| )} | ||
| {showForm && formInfo && patientUuid && patient && !htmlForm && ( | ||
| <ExtensionSlot name="form-widget-slot" state={state} /> | ||
| )} |
There was a problem hiding this comment.
I think the solution to the "losing form data" may be to have a unique workspace for this? Not sure, but I think that's the case.
packages/esm-patient-forms-app/src/htmlformentry/html-form-entry-wrapper.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-forms-app/src/htmlformentry/html-form-entry-wrapper.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-forms-app/src/htmlformentry/html-form-entry-wrapper.component.tsx
Outdated
Show resolved
Hide resolved
packages/esm-patient-forms-app/src/htmlformentry/html-form-entry-wrapper.component.tsx
Outdated
Show resolved
Hide resolved
| border: none; | ||
| margin: 0; | ||
| padding: 0; | ||
| height: 3000px; // TODO: how to make this dynamic? |
There was a problem hiding this comment.
I actually think this is quite hard! Most DOM elements have some way of determining the height of their contents so you could just use fit-content, but iframes are special, because they can load cross-origin content. The best I can think of is adding to the onLoad function so it becomes something like:
const onLoad = useCallback(() => {
const iframe = iframeRef.current;
const dashboard = iframe.contentDocument;
dashboard.querySelector('header')?.remove();
dashboard.querySelector('.patient-header')?.remove();
dashboard.querySelector('#breadcrumbs')?.remove();
iframe.style.display = 'block';
iframe.height = iframe.contentWindow.document.body.scrollHeight;
}, []);
Agreed @ibacher ... we will see, I'm still worried about jinxing ourselves and missing a dealbreaker issue :) Thanks for the helpful comments... I'll let others weigh in and then test them out. |
chibongho
reviewed
Aug 15, 2024
| @@ -82,7 +91,21 @@ const FormEntry: React.FC<FormEntryComponentProps> = ({ | |||
|
|
|||
| return ( | |||
| <div> | |||
| {showForm && formInfo && patientUuid && patient && <ExtensionSlot name="form-widget-slot" state={state} />} | |||
| {showForm && formInfo && patientUuid && patient && htmlForm && encounterUuid && ( | |||
There was a problem hiding this comment.
Maybe simplify to something like this?
const showFormAndLoadedData = showForm && formInfo && patientUuid && patient;
return (
<div>
{showFormAndLoadedData && (
htmlForm?
<ExtensionSlot name="form-widget-slot" state={state} /> :
<HtmlFormEntryWrapper src={encounterUuid? url1 : url2} />
)}
</div>
)
|
No hurry, @denniskigen but would be great to have your thoughts on this as well before I start making the suggested changes. |
…appointment status updated
mogoodrich
commented
Aug 22, 2024
| ); | ||
| }; | ||
|
|
||
| export default HtmlFormEntryWrapper; |
There was a problem hiding this comment.
Thanks for all the tips @ibacher ... the scrollHeight trick generally worked, though I needed to add some padding.
|
Ok, I think this is getting close! Updated screencast here: htnmlformO3.webmSome notes, thoughts:
Anyway ready for re-review @ibacher ... and I'd love your thoughts @denniskigen @brandones |
Makes sense, given the nature of HFE
I don't think it does. I could be wrong about that, but the Visits view is one of the most expensive to render. |
It's a little bit unfortunate that there doesn't seem to be a way to customize the messages here. |
Great, I was hoping that fell under the category of "pre-existing feature" and not something I had to worry about for this PR... :) |
|
This is looking pretty clutch. Thanks, @mogoodrich. Might it be useful to have an error state to show if the form can't load for whatever reason? Or does HFE have an error state that can render correctly within the iframe? |
denniskigen
approved these changes
Aug 23, 2024
It stack traces within the iframe... :) Not the greatest behavior, but not different from what happens in O2 now. |
|
Merging this in as the E2E failure is not related to this change |
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.
Requirements
Summary
Screenshots
Related Issue
Other