AAE-27343 Listen to form rules changes from reactive widgets #10392
Conversation
tomgny
requested review from
VitoAlbano,
BSekula,
mauriziovitale and
wiktord2000
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private updateReactiveFormControlOnFormRulesEvent(instance: any): void { | ||
| instance?.formService.formRulesEvent.pipe(takeUntilDestroyed(this.destroyRef)).subscribe(() => { | ||
| instance?.updateReactiveFormControl(); |
There was a problem hiding this comment.
Can we add an interface to instance? what I mean is adding something like:
interface ReactiveFormWidget {
updateReactiveFormControl: (): void;
formService: Type
}
and implement it in reactive widgets
export class DateTimeWidgetComponent extends WidgetComponent implements OnInit, ReactiveFormWidget {..
So updateReactiveFormControl could be easily traced?
Also we can add some comment to this interface, explaining what it is
There was a problem hiding this comment.
good point, will add it
There was a problem hiding this comment.
If you add an interface (or an abstract class like WidgetComponent) please double check with a custom form widget if it's all good
| this.dateInputControl.setValue(this.field.value, { emitEvent: false }); | ||
| } | ||
|
|
||
| private updateFormControlState(): void { | ||
| this.dateInputControl.setValidators(this.isRequired() ? [Validators.required] : []); |
There was a problem hiding this comment.
Can we use custom validators in date widget? 🤔 or only required
There was a problem hiding this comment.
as default option we use only required from default validators. for other cases, like min/max constraints, we handle it in the handleErrors method, and its needed to be compatible with existing form structure + other non-reactive form widgets.
| @@ -88,14 +92,29 @@ export class FormFieldComponent implements OnInit, OnDestroy { | |||
| instance.fieldChanged.subscribe((field) => { | |||
| if (field && this.field.form) { | |||
| this.visibilityService.refreshVisibility(field.form); | |||
| field.form.onFormFieldChanged(field); | |||
| this.triggerFormFieldChanged(field); | |||
There was a problem hiding this comment.
is it worth to have a method just for a single line of code?
There was a problem hiding this comment.
this method now is used in two places, so thats why i move it to separate method, but i can agree that maybe its not worth
| } | ||
| }); | ||
|
|
||
| if (FormFieldTypes.isReactiveType(instance?.field?.type)) { |
There was a problem hiding this comment.
I was expecting this to be called when something has changed into the form field so inside the fieldChanged stream
There was a problem hiding this comment.
hmmm why? we subscribe to the formRulesEvent when we create this component instance, why do we need to put it under another subscription (nested)? it should listen for these events for the whole lifecycle of the instance
There was a problem hiding this comment.
You don't need to subscribe to the form rules here, you just need to put your part of the code (the if) into the fieldChanged subscription, otherwise other event relying on triggering a change on the form field will fail to update the validation
There was a problem hiding this comment.
if we do not subscribe for formRules changes, we wont be able to update the state of particular instance until the instance.fieldChanged will be triggered (so for example after blur, or input something event).
to make sure I understand you, this is what you mean, right?
| if (FormFieldTypes.isReactiveType(instance?.field?.type)) { | |
| if (componentType) { | |
| this.componentRef = this.container.createComponent(componentType); | |
| const instance = this.componentRef.instance; | |
| instance.field = this.field; | |
| instance.fieldChanged.subscribe((field) => { | |
| if (field && this.field.form) { | |
| this.visibilityService.refreshVisibility(field.form); | |
| this.triggerFormFieldChanged(field); | |
| if (FormFieldTypes.isReactiveType(instance?.field?.type)) { | |
| instance?.updateReactiveFormControl(); | |
| this.triggerFormFieldChanged(instance.field); | |
| } | |
| } | |
| }); |
(i tested it and it doesnt work)
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private updateReactiveFormControlOnFormRulesEvent(instance: any): void { | ||
| instance?.formService.formRulesEvent.pipe(takeUntilDestroyed(this.destroyRef)).subscribe(() => { |
There was a problem hiding this comment.
Reducing the check to only the form rules might miss some other parts where we update the form and the field changed event is triggered
There was a problem hiding this comment.
on each onFieldChanged call, inside the base widget.component.ts , and also on the all events, we update the value of formRulesEvent subject so it will be always triggered for all the events (and its needed for all events, because in modeling, we can set rules for all available event types, like blur, click etc). i think it should be enough, but maybe im not aware of some corner case
|
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
https://hyland.atlassian.net/browse/AAE-27343
What is the new behaviour?
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Other information: