Skip to content
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) support for reusable form components #126

Merged
merged 5 commits into from
Nov 22, 2023

Conversation

pirupius
Copy link
Member

@pirupius pirupius commented Oct 4, 2023

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

This PR introduces logic for support for form components previously in the Angular Form Engine as part of the feature parity milestone. The React form engine already had support for reusing forms but differed in behaviour.

Commonly asked questions?

Qn: What's the difference between sub-forms and form components?
Ans: Both features are built with reuse in mind. But differ in the way they're created, referenced and use cases.

  • Sub-Form: Sub Forms are what the RFE already support with a user able to reference another form. The referenced sub-form is then automatically inserted by the engine and runtime bringing with it any special functionality the subform might have as well content it has as a whole. The user isn't able to exclude questions or sections from the sub-form.

  • Form Components: This on the other hand is a feature that the angular form engine (AFE component docs) already had support for. With this, the user building a form had the power to reuse either part of a form component or all sections of a component. For example if you had a form for Antenatal, you could decide to use only a particular section or questions of the component_art not necessarily the whole component_art form.

Qn: Does this impact my already existing forms with the Ampath JSON schema?
Ans: No. The main goal with this PR for already existing implementations is to provide a smooth transition without having to redesign their forms or at least make the migration less painful. For new implementations, this is an additional tool used to design forms with reuse in mind based on the business needs.

Screenshots

Related Issue

Other

A follow up PR will be made to the O3 form builder to aid faster creation and reuse of form components.

@github-actions
Copy link

github-actions bot commented Oct 4, 2023

Size Change: +344 B (0%)

Total Size: 646 kB

ℹ️ View Unchanged
Filename Size Change
dist/225.js 9.82 kB 0 B
dist/26.js 190 kB 0 B
dist/272.js 6.59 kB -2 B (0%)
dist/294.js 2.63 kB 0 B
dist/366.js 6.94 kB -2 B (0%)
dist/443.js 5.89 kB 0 B
dist/709.js 2.42 kB 0 B
dist/721.js 43.1 kB +350 B (+1%)
dist/787.js 822 B +1 B (0%)
dist/800.js 175 kB -1 B (0%)
dist/932.js 83.6 kB +1 B (0%)
dist/main.js 117 kB +1 B (0%)
dist/openmrs-form-engine-lib.js 3.21 kB -4 B (0%)

compressed-size-action

@pirupius pirupius force-pushed the feat/support-reusable-components branch from 1a59469 to cdafd1b Compare October 12, 2023 01:24
@gracepotma
Copy link
Contributor

This is very interesting, thanks for this draft PR @pirupius. Quick question, sorry if it should be obvious: What's the main difference between a Sub-Form and a Component Form? Is a Component Form specific to a situation where one is embedding a component/mini-app into a bigger form? So unlike a Sub-Form, a Component also brings it's own special functionality, as well as it's own "form" content with it?

CCing @mseaton and @mogoodrich as I believe they've been really interested in embedded components in O3 Forms.

@pirupius pirupius changed the title (feat) support for reusable components (feat) support for reusable form components Oct 18, 2023
@pirupius
Copy link
Member Author

What's the main difference between a Sub-Form and a Component Form? Is a Component Form specific to a situation where one is embedding a component/mini-app into a bigger form? So unlike a Sub-Form, a Component also brings it's own special functionality, as well as it's own "form" content with it?

@gracepotma Some parts of this are true and others are a little interchanged.

Sub-Form: Sub Forms are what is already available with a user able to reference another form. The referenced sub-form is then automatically inserted by the engine and runtime bringing with it any special functionality the subform might have as well content it has as a whole.
In summary, it inserts the subform as is.

Form Components: This on the other hand is a feature that the angular form engine (AFE component docs) supported and with this, the person building a form had the power to reuse either part of a component or all sections of a component. For example if you had a form for Antenatal, you could decide to use only a particular section or questions of the component_art not necessarily the whole component_art form.

Both features are built with reuse in mind. But differ in the way they're created, referenced and use cases.

@pirupius pirupius marked this pull request as ready for review October 23, 2023 05:39
Copy link
Member

@samuelmale samuelmale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @pirupius; however, I have one concern. We seem to be duplicating efforts, especially in this specific task.

A backend module (currently bundled within the refapp) was introduced by @ibacher which defers most build tasks to the backend. This dramatically improves performance for big forms and I believe we should refactor the library to leverage this feature. This module offers the following:

  • All referenced forms loaded into the appropriate parts of the main form
  • All translations provided in a translation file for the appropriate locale
  • Metadata about concepts used in the forms

@kajambiya - This module also plays a role tagential to i18n making life easy to go about adding support for internationalization.


@eudson @kajambiya @pirupius @ibacher what do you think about adding support for "subforms" to this backend module and we get rid of the inbuilt form loader from the library?

src/hooks/useFormComponent.tsx Outdated Show resolved Hide resolved
@kajambiya
Copy link
Collaborator

Details

Thanks for working on this @pirupius; however, I have one concern. We seem to be duplicating efforts, especially in this specific task.

A backend module (currently bundled within the refapp) was introduced by @ibacher which defers most build tasks to the backend. This dramatically improves performance for big forms and I believe we should refactor the library to leverage this feature. This module offers the following:

  • All referenced forms loaded into the appropriate parts of the main form
  • All translations provided in a translation file for the appropriate locale
  • Metadata about concepts used in the forms

@kajambiya - This module also plays a role tagential to i18n making life easy to go about adding support for internationalization.

@eudson @kajambiya @pirupius @ibacher what do you think about adding support for "subforms" to this backend module and we get rid of the inbuilt form loader from the library?

@samuelmale I think this is a nice module. Going to check it out especially in implementing form translations. The idea of using the module to load sub forms also sounds good especially as far as performance is concerned. Once we finish loading the components using the module, we shall look into expanding the model to add functionality for loading sub forms.

@kajambiya
Copy link
Collaborator

@pirupius You have some lint and type checking issues.

@ibacher
Copy link
Member

ibacher commented Oct 30, 2023

what do you think about adding support for "subforms" to this backend module and we get rid of the inbuilt form loader from the library?

@samuelmale @pirupius @eudson In principle, I'm onboard with bringing whatever functionality we can into the backend. However, I think there are some things we should consider here:

  • What is the future goal we want; I think having both "references" and "sub-forms" is confusing. When, for example, would I use a "subform" instead of a reference to a page and vice versa?
  • What is the "special functionality" that a "subform" might have?
  • How are "subform" pages ordered with respect to the other pages?

Last point: I'd probably be happier with this if we can accomodate the "subform" terminology to something like the "reference" system. I realise that's a breaking change for OHRI forms, but that way, we avoid having two user-facing features were are a bit hard to disentangle. Ideally, we end up with a unified, intuitive single feature for including content from other forms in a single form and then for backwards compatibility, we can support "subform" as a synonym for some kind of reference with similar semantics.

@eudson
Copy link
Contributor

eudson commented Oct 31, 2023

what do you think about adding support for "subforms" to this backend module and we get rid of the inbuilt form loader from the library?

@samuelmale @pirupius @eudson In principle, I'm onboard with bringing whatever functionality we can into the backend. However, I think there are some things we should consider here:

  • What is the future goal we want; I think having both "references" and "sub-forms" is confusing. When, for example, would I use a "subform" instead of a reference to a page and vice versa?
  • What is the "special functionality" that a "subform" might have?
  • How are "subform" pages ordered with respect to the other pages?

Last point: I'd probably be happier with this if we can accomodate the "subform" terminology to something like the "reference" system. I realise that's a breaking change for OHRI forms, but that way, we avoid having two user-facing features were are a bit hard to disentangle. Ideally, we end up with a unified, intuitive single feature for including content from other forms in a single form and then for backwards compatibility, we can support "subform" as a synonym for some kind of reference with similar semantics.

@ibacher I think we will have to go with the suggestion of your last sentence. Sub-form it is not just a reusable component as the reference componentes are. When referencing sub-forms one can post data into multiple (different) encounterTypes. An example in OHRI is the COVID Assessment form, it has an embedded form for lab orders (not using the order model yet) that is used to do a lab order if certain conditions on the form are met. Another example is within HTS where if the client is HIV positive you want to do the enrolment right away, with sub-forms (I would call it embedded forms) you can do this seamlessly and have the data posted into two different encounters: HIV Testing, ART Enrolment. This is just to mention a few examples.

The Uganda team also wants to leverage this functionality. I believe this could have been also achieved with workflows but that feature is still not available in O3. cc: @slubwama

Another cool thing of sub-forms is that they can also post to the same encounterType if you pass the right configs, right @samuelmale @kajambiya ? Which can essentially be considered a referenced component and could be expanded to become one, the only thing would be adding the cool stuff the AFE has where you can choose which parts to display and where.

Regarding supporting all of this from the backend and have all cooked when loading into the frontend I think it is a good idea and worths exploring.

Can I propose we have a technical meeting about the whole form-engine architecture sooner than later because I keep seeing this comments on PRs and I think it would be beneficial if we agree on certain aspects before the code is written, @pirupius has spent a lot of time on this trying to figure out the best approach

@pirupius pirupius force-pushed the feat/support-reusable-components branch from b75ed87 to 62d521c Compare November 9, 2023 06:01
@@ -24,7 +24,7 @@ export function evaluateExpression(
if (!expression?.trim()) {
return null;
}
const allFieldsKeys = fields.map(f => f.id);
const allFieldsKeys = fields.map((f) => f.id);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file doesn't have any code changes. Only formatting

@@ -47,7 +47,7 @@ export function getForm(

export function loadSubforms(parentForm) {
parentForm.pages = parentForm.pages || [];
parentForm.pages.forEach(page => {
parentForm.pages.forEach((page) => {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same applies to this file. Only format changes

* Functions to support reusable Form Components
*/
function getReferencedForms(formJson: OHRIFormSchema): Array<ReferencedForm> {
const referencedForms: Array<any> = formJson.referencedForms;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what happens when formJson is undefined ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has been handled with the typings and validation on the call

src/hooks/useFormJson.tsx Show resolved Hide resolved
@pirupius
Copy link
Member Author

pirupius commented Nov 17, 2023

I've created a ticket to explore migration to o3forms module as well as spec'ng migration of subform to references

@pirupius
Copy link
Member Author

@samuelmale @kajambiya please take another pass at this. If there no objections, will proceed and merge it and build on top of it for the other enhancements.

Copy link
Collaborator

@kajambiya kajambiya left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@pirupius
Copy link
Member Author

Thanks @kajambiya
Will go ahead and merge this. Post-merge reviews are welcome.

@pirupius pirupius merged commit 06b2746 into main Nov 22, 2023
6 checks passed
@pirupius pirupius deleted the feat/support-reusable-components branch November 22, 2023 06:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants