feat(#7462) add cht-form web component for standalone Enketo form support #8153
Conversation
jkuester
force-pushed
the
7462_forms_module
branch
from
384c0c0
to
52ec7e8
Compare
jkuester
force-pushed
the
7462_forms_module
branch
from
afae115
to
c8d06d4
Compare
|
SonarCloud Quality Gate failed.
|
Component works to load hard-coded form! Add enketo.component code to new component Include z-score stub service! Component builds with EnektoForm included! Add TranslateModule to cht-form app.module Basic component working (without Enekto) feat: clean up stubs and tsconfig feat: add stub services to get it to build! Add initial code for cht-form component (still broken) Initial config for cht-forms-component. Things build, but nothing happens
Add some documentation for how to customize for different layout.
jkuester
force-pushed
the
7462_forms_module
branch
from
50f5b69
to
c50da99
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
# Conflicts: # tests/page-objects/default/enketo/generic-form.wdio.page.js # tests/page-objects/default/enketo/pregnancy.wdio.page.js # webapp/src/ts/modules/contacts/contacts-edit.component.ts # webapp/tests/karma/ts/modules/contacts/contacts-edit.component.spec.ts
|
@garethbowen @dianabarsan FYI, I think we are pretty close to getting this landed! Ultimately we have not really deviated at all from the design discussed in #7462, so I feel pretty good about what have have! That being said, I did want to circle back with you for a final check of the overall structure since this is breaking some new ground in our code base. Let me know if there is anything here you find concerning or you think could be done better! Thanks! |
There was a problem hiding this comment.
Amazing.
I've added a few nitpick comments. The main question I have relates to whether we componentise the css and js and then actually use the web component in CHT webapp. It's probably too much work for right now, but curious if you think that's where we should plan to end up...
| @@ -0,0 +1,29 @@ | |||
| <div class="reports"> | |||
| <div class="content"> | |||
| <div class="page" style="top: 0"> | |||
There was a problem hiding this comment.
I feel like we're using the web component backwards... ie: the web component would usually define the CSS it needs and then be included in webapp which overwrites and extends with specific CSS. This comment implies that the web component is bringing in the entire webapp CSS and then overwriting it back to stock. Is that right? Is this because splitting out the CSS would be a big lift so we're leaving it for now?
|
@garethbowen for the sake of visibility, I wanted to address your comment regarding if the webapp should just be refactored to use the cht-form component here at the top level (since I think this may have relevance to future discussions on the subject). TLDR is yes, we have avoided splitting out css or refactoring the rest of the webapp to depend on the cht-form component for the sake of time. The goal was to start here with an MVP that had enough functionality to be useful to the test-harness, but also be generic enough to leave the door open to future reuse in the CHT, itself (as well as other places). This approach was first considered back on the issue thread. At that time, I said that having webapp depend on cht-form:
Most of that rational still holds, but we know a lot more now then we did back then about how this is all going to fit together It might be possible for the CHT webapp code to depend on cht-form as an Angular componentOriginally, I was thinking that for webapp to depend on cht-form, the only option would be to have webapp consume the built cht-form web component (and consume the js/css artifacts). This would be "best" from an encapsulation perspective, but would perhaps make things a lot more confusing from a code-reuse perspective (if cht-form depended on the same Angular service as the other CHT webapp code, then I guess you might end up with duplicate copies of the service code??? 🤔 ). As we have progressed in this project, though, I now think it might be possible to have a single component that both can be built into a web component as well as be integrated as a normal Angular component into the rest of the webapp code. I have not tested this yet, though. Even if this works, it would not really solve all our problems here. We would still have to be careful with what the cht-form component depended on to make sure we are not pulling too much into the web component. Also, I do not think it would solve/simplify the css challenges. Ultimately, I am still undecided regarding if we even want to use the cht-form component as a normal Angular component, or if it would be better in the long term to just depend on the built cht-form artifacts.... Despite all of the refactoring work, there is still some tightly coupled code in webapp around formsThings have been vastly improved, but in an effort to reduce the code-change footprint, we have minimized any changes to the code that consumes the enketo/form services. The CHT webapp enketo component is a no-logic wrapper for displaying Enketo forms. It is getting used to display contact/app/training forms, but those other components all have custom logic that now leverages the Form service in different ways to achieve their desired behavior. The cht-form component, on the other hand, is an opinionated wrapper for Enketo forms that handles interacting with the underlying Enekto and ContactSave services directly. I think the fault-lines in the code are now more clear than ever and even this discussion is evidence that we can see how cht-form might start to fit in with the rest of the CHT webapp code. However, the cht-form is still pretty far from being a drop-in replacement for the current enekto component. At the end of the day, I feel like we have made a lot of positive progress here and the cht-form component should be reasonably maintainable even just as a spin-off of the main webapp code-base. While I am sure there are ways to further improve this structure (up to and including a refactoring that makes the CHT webapp code depend on cht-form) it is not yet obvious to me exactly what is the best way to proceed (or even what benefits would actually be realized from that work)... |
# Conflicts: # .github/workflows/build.yml
This reverts commit 7c5c63d.
…ts for the default folder
…8648) This commit adds new automated tests for the cht-form web component. It covers all the available forms for the standard config: * child_health_registration * death_confirmation * delivery * immunization_visit * postnatal_visit * pregnancy * pregnancy_visit * health_center-create * health_center-edit
Co-authored-by: Mokhtar <[email protected]>
|
@m5r I am off the rest of the week. I responded to your comments above, and if you agree with leaving the code as-is, can you go ahead and merge this to |
Closes #7462
See the issue for more context/commentary, but ultimately this PR adds configuration and a minimal amount of code into
webappto allow for generating a custom web component that will allow for rendering/interaction with CHT forms outside of a formal CHT instance. The web component is built from the existing Angular code in webapp, but is packaged into.js/.cssfiles in such a way that it can be consumed externally by non-Angular webpages.See medic/cht-conf-test-harness#245 for an example of how this web component will be used!
Also see https://github.com/medic/support-scripts/pull/12 for a super simple web page that uses the web component!
contact-save service refactor
In the previous refactoring of the EnketoService/FormService (#8455) I missed the
contact-save.servicecode. This service owns all of the logic for saving contact forms (theform.serviceis used for rendering contact and app forms and for saving app forms). The Web Component needs to depend on thecontact-saveservice logic for processing the submitted contact form data into the expected contact documents. However we do not want to also depend on the logic in thecontact-saveservice for doing additional CHT things like running the client-side transitions.So, I have refactored the
contact-saveservice to move all the CHT-specific logic to theform.service(similar to what we did with theenketo.servicecode). Existing webapp code now will use theform-servicefor rendering/saving both app and contact forms. Then, the Web Component can depend directly on thecontact-save.service(as well as theenketo.service) without pulling in a ton of extra dependencies.(Note that as with the EnketoService/FormService refactor, I have tried to achieve the best balance of high unit test coverage vs spending days refactoring the tests. So, largely I duplicated the existing
contact-savetests into theforms.servicetest file and made sure they all passed. Then I did my best to de-dupe the tests between thecontact-saveand theforms.servicetest file so there was not too much redundancy. However, I did not update the newforms.servicetests (the ones I copied in) to mock thecontact-saveservice. For the sake of time, I left them as they were in thecontact-saveservice test file. This is not ideal, but I think is an acceptable trade-off...)Caveats
@tatilepizs is adding proper e2e tests in a separate PR that will eventually be merged into this one. So, what you currently see in thetestsdirectory just represents a prototype of future tests.The contents ofwebapp/web-componenets/exampleare also just a prototype of a site that actually uses the web component. This directory will not be included when merging tomaster, but I am keeping it in here for now because it is useful when manually testing the web component.Code review checklist
Compose URLs
If Build CI hasn't passed, these may 404:
License
The software is provided under AGPL-3.0. Contributions to this project are accepted under the same license.