-
Notifications
You must be signed in to change notification settings - Fork 268
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
AAE-27343 Listen to form rules changes from reactive widgets #10392
AAE-27343 Listen to form rules changes from reactive widgets #10392
Conversation
} | ||
} | ||
} | ||
} | ||
|
||
private updateReactiveFormControlOnFormRulesEvent(instance: any): void { | ||
instance?.formService.formRulesEvent.pipe(takeUntilDestroyed(this.destroyRef)).subscribe(() => { | ||
instance?.updateReactiveFormControl(); |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, will add 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use custom validators in date widget? 🤔 or only required
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it worth to have a method just for a single line of code?
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
this.datetimeInputControl.setValue(this.field.value, { emitEvent: false }); | ||
} | ||
|
||
private updateFormControlState(): void { |
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.
private updateFormControlState(): void { | |
private updateFormControlValidators(): void { |
WDYT?
To me state is really broad term
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 state in this case better describe what this method does, because it updates also readonly/disabled state (required is also state of the field)
7da876c
to
bd2b94d
Compare
82cb763
to
50ad33c
Compare
50ad33c
to
e122795
Compare
e122795
to
7935d17
Compare
Quality Gate failedFailed conditions |
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: