AAE-23950 Apply reactive forms for date&time and date widgets #9852
Conversation
tomgny
force-pushed
the
fix/AAE-19102-date-should-be-formatted-correctly-on-task-form
branch
from
37da97b to
409ae4a
Compare
tomgny
force-pushed
the
fix/AAE-19102-date-should-be-formatted-correctly-on-task-form
branch
3 times, most recently
from
98400b8 to
9b99a17
Compare
tomgny
force-pushed
the
fix/AAE-19102-date-should-be-formatted-correctly-on-task-form
branch
from
3a31bf4 to
3cf660d
Compare
tomgny
force-pushed
the
fix/AAE-19102-date-should-be-formatted-correctly-on-task-form
branch
from
4ae491c to
4728515
Compare
tomgny
requested review from
eromano,
popovicsandras and
DenysVuika
as code owners
| minDate: Date; | ||
| maxDate: Date; | ||
| datetimeInputControl: FormControl<Date>; |
There was a problem hiding this comment.
either provide default values for the properties or declare them with ?
There was a problem hiding this comment.
we need to define this control at the ngOnInit level, because then we have access to the initial value which is the default value of the control. so in that case, shouldn't we use exclamation mark ! instead making it opitonal by ??
There was a problem hiding this comment.
You can define the control here and patch it in the ngOnInit when the value is defined.
Also actually it's another problem, because afaik, value is an input. So to make it 100% we should patch the form every time the value is changed.... given that the base implementation is correct 😅
There was a problem hiding this comment.
defined at declaration level, and added patch in the onInit.
about onChange, i think its not needed (i checked it), because we don't update this.field during component lifecycle, so ngOnChanges is not called for this input. Its only used to initialize default values
lib/core/src/lib/form/components/widgets/date-time/date-time.widget.ts
Outdated
Show resolved
Hide resolved
| const dateAdapter = this.dateAdapter as AdfDateFnsAdapter; | ||
| dateAdapter.displayFormat = this.field.dateDisplayFormat; | ||
|
|
||
| const dateTimeAdapter = this.dateTimeAdapter as AdfDateTimeFnsAdapter; |
There was a problem hiding this comment.
Instead of type casting maybe you can define the type when you inject DateTimeAdapter?
There was a problem hiding this comment.
currently, when we add the type at the inject adapter level, we get this error:
Type 'DateAdapter<any, any>' is missing the following properties from type 'AdfDateFnsAdapter': displayFormat, formatsts(2739)
so for now, i would prefer to skip additional refactoring around these adapters, we can consider it as a future improvement. is it ok?
Issue -> https://hyland.atlassian.net/browse/AAE-24763
|
|
||
| private handleErrors(errors: ValidationErrors): void { | ||
| const errorAttributes = new Map<string, string>(); | ||
| switch (true) { |
There was a problem hiding this comment.
For me it would be clearer to use if else syntax here rather than switch(true), but it's a personal preference so the change is up to you
There was a problem hiding this comment.
i think in the future we can implement general mat-errors handler component, that should handle&display errors. For now, we use the validationSummary and custom error-widget component. So once we implement the new component, we can get rid of this code from each widgets.
Issue -> https://hyland.atlassian.net/browse/AAE-24762
| @@ -421,6 +422,7 @@ export class FormCloudComponent extends FormBaseComponent implements OnChanges, | |||
| this.displayModeOn.emit(this.displayModeService.findConfiguration(this.displayMode, this.displayModeConfigurations)); | |||
| } | |||
|
|
|||
| this.changeDetector.detectChanges(); | |||
There was a problem hiding this comment.
Why do you need to call the detectChanges() here?
There was a problem hiding this comment.
currently, in case where some field is empty and required at the initial state, we need to update the form state. Because its initial state and no action was performed, form doesn't now that empty required field is invalid, so we need to do it manually at the widgets level (in the ngOnInit). And because we need to do it manually, we change the state after it was checked and we get the Expression has been change after it was checked error. It should be solved once we move the whole form to be reactive instead of manipulate it manually as we currently do.
| @@ -78,7 +76,6 @@ import { FormCloudSpinnerService } from './services/spinner/form-cloud-spinner.s | |||
| DropdownCloudWidgetComponent, | |||
| RadioButtonsCloudWidgetComponent, | |||
| AttachFileCloudWidgetComponent, | |||
| DateCloudWidgetComponent, | |||
There was a problem hiding this comment.
Can we register a Jira ticket to make all form cloud widgets standalone?
There was a problem hiding this comment.
lib/process-services-cloud/src/lib/form/components/widgets/date/date-cloud.widget.ts
Outdated
Show resolved
Hide resolved
lib/process-services-cloud/src/lib/form/components/widgets/date/date-cloud.widget.ts
Outdated
Show resolved
Hide resolved
lib/process-services-cloud/src/lib/form/components/widgets/date/date-cloud.widget.ts
Outdated
Show resolved
Hide resolved
tomgny
changed the title
|
tomgny
deleted the
fix/AAE-19102-date-should-be-formatted-correctly-on-task-form
branch
…ring specified display format
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)
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: