-
Notifications
You must be signed in to change notification settings - Fork 38
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: update forms doc #1694
base: main
Are you sure you want to change the base?
feat: update forms doc #1694
Conversation
☁️ Nx Cloud ReportCI is running/has finished running commands for commit 2332915. As they complete they will appear below. Click to see the status, the terminal output, and the build insights. 📂 See all runs for this CI Pipeline Execution ✅ Successfully ran 1 targetSent with 💌 from NxCloud. |
packages/@o3r/forms/README.md
Outdated
@@ -12,7 +12,11 @@ This package is an [Otter Framework Module](https://github.com/AmadeusITGroup/ot | |||
[![Stable Version](https://img.shields.io/npm/v/@o3r/forms?style=for-the-badge)](https://www.npmjs.com/package/@o3r/forms) | |||
[![Bundle Size](https://img.shields.io/bundlephobia/min/@o3r/forms?color=green&style=for-the-badge)](https://www.npmjs.com/package/@o3r/forms) | |||
|
|||
This module provides utilities to enhance Angular form (asynchronous decorator, additional validator, error store...). | |||
This module provides utilities to enhance the build of Angular reactive forms in the Otter context. These utilities include: |
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.
From what I see this entire module is related to guidelines for a product application, with container/presenter structure for components.
I think we should explain this here in the beginning and maybe add another section where to say that for small apps, be-spoke apps (I don;t know how to name them :D) this module does not bring added value on top of angular forms.
Maybe we can have a discussion inside the team to double check if something from this module can be used in any use-case.
Any thoughts @pginoux-1A @kpanot ?
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.
I agree with you for all the complexity brings by container / presenter.
But some features could be interesting even with as small apps
for example if you want to handle the submission at page level and display the error messages outside the form component
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.
Also, we should probably avoid "in the Otter context", from an outside perspective this is really scary. If I don't know what Otter is, I would imagine a big black box with guidelines from an entity I know nothing about.
Unlike Angular, ware not known enough to use this kind of sentence in a public documentation.
I would replace it with the actual context (described by @matthieu-crouzet and @mrednic-1A ) on where our utilities can be useful.
docs/forms/FORM_ERRORS.md
Outdated
- __longTranslationKey__ used for a more detailed message on the same error | ||
- __translationParams__ translations parameters | ||
- __validationError__ original error object | ||
- `ErrorMessageObject` is associated to an error message on a field. It contains: |
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.
Shouldn't this kind of doc be in the tsdoc only ?
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.
It depends, on the usage, it is nice to see how to use this model without going through the code. But maybe it could be more interesting to see how to use it instead of writing a tsdoc.
docs/forms/FORM_ERRORS.md
Outdated
|
||
<a name="customerrors"></a> | ||
We have to make sure that we provide the `htmlElementId` of the errors in the store that match the __HTML fields__. | ||
For this, the presenter receives an `id` as input and for each field we are concatenating, the `id` with the `formControlName`. |
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.
We anymore generate components with a default id, overridable by input.
For this, the presenter receives an `id` as input and for each field we are concatenating, the `id` with the `formControlName`. | |
To identify a field, we can generate an `id` in the container instance, provide it through the input mechanism, and concatenate it with the `formControlName`. |
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.
maybe we could remove references to the container?
docs/forms/FORM_ERRORS.md
Outdated
<a name="customerrors"></a> | ||
We have to make sure that we provide the `htmlElementId` of the errors in the store that match the __HTML fields__. | ||
For this, the presenter receives an `id` as input and for each field we are concatenating, the `id` with the `formControlName`. | ||
Since the container sets a __unique id__, we are sure to have unique HTML identifiers for the form fields. |
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.
To complement the comment just above
Since the container sets a __unique id__, we are sure to have unique HTML identifiers for the form fields. | |
Since we will have a __unique id__ by instance of the container, we are sure to have unique HTML identifiers for the form fields. |
docs/forms/FORM_ERRORS.md
Outdated
For the localization of the error messages we keep the same way we have today ([LOCALIZATION](../localization/LOCALIZATION.md)), but we have specific places where to define the default translations of error messages. | ||
<a name="translationcustomerror"></a> | ||
For the localization of the error messages, we keep the same way of working as we have today (check out [LOCALIZATION](../localization/LOCALIZATION.md)), | ||
but we have specific places where to define the default translations of error messages. |
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.
Where is this specific places ?
I think the place is the same as all others localization key.
maybe we can remove this sentence.
docs/forms/FORM_STRUCTURE.md
Outdated
__Presenter class:__ | ||
* Here we have to create the `formGroup`/`formArray`/`formControl` object. | ||
* Provide [NG_VALUE_ACCESSOR](https://angular.io/api/forms/NG_VALUE_ACCESSOR) - used to provide a [ControlValueAccessor](https://angular.io/api/forms/DefaultValueAccessor) for form controls, to write a value and listen to changes on input elements. | ||
* Provide [NG_VALIDATORS](https://angular.io/api/forms/NG_VALIDATORS) - this is an [InjectionToken](https://angular.io/api/core/InjectionToken) for registering additional synchronous validators used with forms. | ||
```typescript | ||
// in presenter class | ||
@Component({ |
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.
Should we keep an example like that in the doc
or have it in the showcase and add a link to it here
packages/@o3r/forms/README.md
Outdated
@@ -12,7 +12,11 @@ This package is an [Otter Framework Module](https://github.com/AmadeusITGroup/ot | |||
[![Stable Version](https://img.shields.io/npm/v/@o3r/forms?style=for-the-badge)](https://www.npmjs.com/package/@o3r/forms) | |||
[![Bundle Size](https://img.shields.io/bundlephobia/min/@o3r/forms?color=green&style=for-the-badge)](https://www.npmjs.com/package/@o3r/forms) | |||
|
|||
This module provides utilities to enhance Angular form (asynchronous decorator, additional validator, error store...). | |||
This module provides utilities to enhance the build of Angular reactive forms in the Otter context. These utilities include: |
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.
Also, we should probably avoid "in the Otter context", from an outside perspective this is really scary. If I don't know what Otter is, I would imagine a big black box with guidelines from an entity I know nothing about.
Unlike Angular, ware not known enough to use this kind of sentence in a public documentation.
I would replace it with the actual context (described by @matthieu-crouzet and @mrednic-1A ) on where our utilities can be useful.
packages/@o3r/forms/README.md
Outdated
This module provides utilities to enhance the build of Angular reactive forms in the Otter context. These utilities include: | ||
* An asynchronous decorator (`@AsyncInput`) to ensure subscriptions are handled if the references of the input observables change. | ||
* Basic and custom validators to validate user input for accuracy and completeness. | ||
* A dedicated NgRX store for form errors to have the possibility of displaying error messages. |
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.
* A dedicated NgRX store for form errors to have the possibility of displaying error messages. | |
* A dedicated NgRX store for form errors to have the possibility of displaying error messages outside the form component. |
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.
Not sure if this is useful to mention...
docs/forms/FORM_ERRORS.md
Outdated
# Form errors | ||
|
||
Handling the form errors in Otter context (container/presenter, localization ...), it's a bit different from creating a form in a component and do all the logic there. | ||
<a name="store"></a> | ||
Handling the form errors in the Otter context (container/presenter, localization, etc.) is a bit different from creating a form in a component and doing all the logic there. |
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.
Why is the way of handling errors in the Otter context different? And is it really true that this is only for an Otter context? This might be a bit scary when we actually just want to share some guidelines and utilities to help developer solve some architectural challenges?
docs/forms/FORM_ERRORS.md
Outdated
- __ElementError__ | ||
This object contains all the errors associated to the html element. | ||
The identifier __htmlElementId__ can be used as an anchor link to focus on the html element on which the validation has failed | ||
- The `ElementError` object contains all the errors associated to the HTML element. |
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.
Make sure to introduce the ElementError if you remove the code above. If not, please ignore this message :)
docs/forms/FORM_ERRORS.md
Outdated
We have to make sure that we are providing the __htmlElementId__ for the errors in the store which is matching the __html field__. | ||
For this, the presenter is receiving an __id__ as input and for each field we are concatenating the __id__ with the __formControlName__. As the container is setting a __unique id__ we are sure that we have uniques html ids for the form fields. | ||
The object returned by the __validate__ is the error object which is propagated to the container. | ||
The presenter has to implement the [Validator](https://angular.io/api/forms/NG_VALIDATORS) or the [AsyncValidator](https://angular.io/api/forms/NG_ASYNC_VALIDATORS) interface |
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.
If we want to leave out the presenter/container structure, maybe we should instead refer to the presenter here as the input element, or the ControlValueAccessor?
docs/forms/FORM_ERRORS.md
Outdated
This one is using _customErrors_ key with an array of __ErrorMessageObject__ which has to contain all the custom errors for a form control or group. | ||
There are two types of validators (see [Form Validation](./FORM_VALIDATION.md)) and therefore two categories of error messages: | ||
|
||
- __Custom error__ - set in the container |
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.
I think it could be worth to add something about why setting an error in the presenter or in the container.
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.
In the form validation file, the difference between the two validators is explained, should I re explain the difference between the two errors here?
4a3fd6e
to
33973dd
Compare
0d41e02
to
e1d4224
Compare
public translations: FormsPresTranslation; | ||
|
||
/** Observable used to notify the component that a submit has been fired from the page */ | ||
public submitTrigger$: Observable<boolean>; |
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 has to be an @input @AsyncInput received from the page component in order to notify this component to trigger a submit. I am assuming here that we have a button on the page component which triggers the submit, not on this component.
With the button on this component we don't need the observables to handle the submit, we call directly submitAction.
But I think we want to showcase the complex example so I would move the button at page level and transform the submitTrigger in an Input.
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 button is in this component, and not on the page.
To explain the structure, I have a page which contains the forms component (this current component), which contains two subcomponents (forms-personal-info
and forms-emergency-contact
) each having a form. The submit button is in the forms component, but if you prefer I can make the structure more complex and move the button to the page.
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.
If the button is in this component, then it would be enough to call 'submitAction' method when clicking on the button, no ? (no need of the observables)
console.log('FORMS COMPONENT: emergency contact form status', this.emergencyContactFormControl.status); | ||
// eslint-disable-next-line no-console | ||
console.log('FORMS COMPONENT: submit logic here', this.personalInfoFormControl.value, this.emergencyContactFormControl.value); | ||
} |
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.
Could you create an issue to track the errors
case with the example of the store usage ?
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.
Would you like me to update the PR and add the store to track the errors? Or would you prefer for me to create an issue?
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.
another issue would be cleaner imo.
apps/showcase/src/components/showcase/forms/forms-pres.component.ts
Outdated
Show resolved
Hide resolved
if (isValid) { | ||
this.submittedFormValue = JSON.stringify(this.personalInfoFormControl.value) + '\n' + JSON.stringify(this.emergencyContactFormControl.value); | ||
// eslint-disable-next-line no-console | ||
console.log('FORMS COMPONENT: personal info form status', this.personalInfoFormControl.status); |
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.
You should probably leave the submitForm logic to each form and just call submitPersonalInfoForm and the other one.
This would highlight that you do not need to duplicate each blocks submit action logic but let them handle it.
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.
I would have prefer two different component to really highlight each component is the owner of its business logic but we can keep it this way if you think that would be overkill
apps/showcase/src/components/showcase/forms/forms-pres.component.ts
Outdated
Show resolved
Hide resolved
...e/src/components/utilities/forms-emergency-contact/forms-emergency-contact-pres.component.ts
Outdated
Show resolved
Hide resolved
docs/forms/README.md
Outdated
> The first subcomponent has a form for the user to fill out their personal information and the second subcomponent has a form for the user to fill out | ||
> information about their emergency contact. In this example, the two subcomponents correspond to input components. | ||
> | ||
> You can find the implementation of this forms example [here](../../apps/showcase/src/components/showcase/forms). |
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.
> You can find the implementation of this forms example [here](../../apps/showcase/src/components/showcase/forms). | |
> You can find the implementation of this forms example [in the showcase code source](../../apps/showcase/src/components/showcase/forms). |
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.
Also do we support relative link to the application (the documentation is published on a separated url)
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.
No that's a good point, I put a relative link since the code was not published yet but I will update it to the URL of the component.
docs/forms/FORM_STRUCTURE.md
Outdated
Container/presenter architecture was put in place to ensure the best re-usability/sharing | ||
<a name="form-creation"></a> | ||
### Form creation in container or in presenter? | ||
This documentation will help you with some best practices to use when building Angular reactive forms components that have a parent/input component structure (such as container/presenter). |
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.
Maybe mention this is in fact only an implementation of the ControlValueAccessor pattern but we chose to guide them with this example
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.
(and that we cover as well the errors but I guess this is in another section :) )
Proposed change
Update documentation on forms