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
fix(Time Value Validators): Propagate valueRequiredValidator to child component (DSP-1218) #257
Changes from 3 commits
e228c80
b0aca28
6c0280d
a210b88
724d42d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,11 @@ | ||
<div [formGroup]="form"> | ||
<mat-form-field class="child-input-component"> | ||
<dsp-jdn-datepicker [activeCalendar]="'Gregorian'"> | ||
<input matInput class="time-input-element date" [matDatepicker]="picker" [formControlName]="'date'" (dateChange)="_handleInput()" aria-label="Date" [placeholder]="dateLabel" readonly> | ||
<mat-datepicker-toggle matSuffix [for]="picker"></mat-datepicker-toggle> | ||
<mat-datepicker #picker></mat-datepicker> | ||
</dsp-jdn-datepicker> | ||
</mat-form-field> | ||
<dsp-jdn-datepicker [activeCalendar]="'Gregorian'"> | ||
<mat-form-field class="child-input-component"> | ||
<input matInput class="time-input-element date" [matDatepicker]="picker" [formControlName]="'date'" (dateChange)="_handleInput()" aria-label="Date" [placeholder]="dateLabel" readonly> | ||
<mat-datepicker-toggle matSuffix [for]="picker"></mat-datepicker-toggle> | ||
<mat-datepicker #picker></mat-datepicker> | ||
</mat-form-field> | ||
</dsp-jdn-datepicker> | ||
<mat-form-field class="child-input-component"> | ||
<input matInput class="time-input-element time" [formControlName]="'time'" type="time" aria-label="Time" (input)="_handleInput()" [placeholder]="timeLabel" [errorStateMatcher]="matcher"> | ||
<mat-error *ngIf="form.controls.time.hasError('required')"> | ||
|
@@ -15,4 +15,10 @@ | |
<span class="custom-error-message">Time should be given in precision HH:MM.</span> | ||
</mat-error> | ||
</mat-form-field> | ||
<div class="date-form-error"> | ||
<mat-error *ngIf="((dateFormControl.hasError('validDateTimeRequired') || timeFormControl.hasError('validDateTimeRequired')) && | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it would be easier to understand this if the null checks come first. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in a210b88 |
||
(dateFormControl.value !== null || timeFormControl.value !== null))"> | ||
<span class="custom-error-message">A time value must have a <strong>date</strong> and <strong>time</strong>.</span> | ||
</mat-error> | ||
</div> | ||
</div> |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,7 +2,7 @@ import { async, ComponentFixture, TestBed } from '@angular/core/testing'; | |
|
||
import { TimeInputComponent, DateTime } from './time-input.component'; | ||
import { Component, OnInit, ViewChild, DebugElement } from '@angular/core'; | ||
import { FormGroup, FormBuilder, ReactiveFormsModule } from '@angular/forms'; | ||
import { FormGroup, FormBuilder, ReactiveFormsModule, FormControl } from '@angular/forms'; | ||
import { MatFormFieldModule } from '@angular/material/form-field'; | ||
import { MatInputModule } from '@angular/material/input'; | ||
import { BrowserAnimationsModule } from '@angular/platform-browser/animations'; | ||
|
@@ -42,6 +42,35 @@ class TestHostComponent implements OnInit { | |
} | ||
} | ||
|
||
/** | ||
* Test host component to simulate parent component. | ||
*/ | ||
@Component({ | ||
template: ` | ||
<div [formGroup]="form"> | ||
<mat-form-field> | ||
<dsp-time-input #timeInput [formControlName]="'time'" [valueRequiredValidator]="false"></dsp-time-input> | ||
</mat-form-field> | ||
</div>` | ||
}) | ||
class NoValueRequiredTestHostComponent implements OnInit { | ||
|
||
@ViewChild('timeInput') timeInputComponent: TimeInputComponent; | ||
|
||
form: FormGroup; | ||
|
||
constructor(private _fb: FormBuilder) { | ||
} | ||
|
||
ngOnInit(): void { | ||
|
||
this.form = this._fb.group({ | ||
time: new FormControl(null) | ||
}); | ||
|
||
} | ||
} | ||
|
||
describe('TimeInputComponent', () => { | ||
let testHostComponent: TestHostComponent; | ||
let testHostFixture: ComponentFixture<TestHostComponent>; | ||
|
@@ -132,4 +161,62 @@ describe('TimeInputComponent', () => { | |
expect(dateTime.time).toEqual('12:00'); | ||
|
||
}); | ||
|
||
it('should mark the form\'s validity correctly', () => { | ||
expect(testHostComponent.timeInputComponent.valueRequiredValidator).toBe(true); | ||
expect(testHostComponent.timeInputComponent.form.valid).toBe(true); | ||
|
||
testHostComponent.timeInputComponent.timeFormControl.setValue(null); | ||
|
||
testHostComponent.timeInputComponent._handleInput(); | ||
|
||
expect(testHostComponent.timeInputComponent.form.valid).toBe(false); | ||
|
||
testHostComponent.timeInputComponent.timeFormControl.setValue(""); | ||
|
||
testHostComponent.timeInputComponent._handleInput(); | ||
|
||
expect(testHostComponent.timeInputComponent.form.valid).toBe(false); | ||
}); | ||
}); | ||
|
||
describe('TimeInputComponent no value required', () => { | ||
let testHostComponent: NoValueRequiredTestHostComponent; | ||
let testHostFixture: ComponentFixture<NoValueRequiredTestHostComponent>; | ||
|
||
beforeEach(async(() => { | ||
TestBed.configureTestingModule({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you need this setup again? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is how it's done in the other complex value spec files. It could possibly be refactored at a later point but I don't think it's necessary. |
||
imports: [ | ||
ReactiveFormsModule, | ||
MatFormFieldModule, | ||
MatInputModule, | ||
MatDatepickerModule, | ||
MatJDNConvertibleCalendarDateAdapterModule, | ||
BrowserAnimationsModule], | ||
declarations: [TimeInputComponent, NoValueRequiredTestHostComponent, JDNDatepickerDirective] | ||
}) | ||
.compileComponents(); | ||
})); | ||
|
||
beforeEach(() => { | ||
testHostFixture = TestBed.createComponent(NoValueRequiredTestHostComponent); | ||
testHostComponent = testHostFixture.componentInstance; | ||
testHostFixture.detectChanges(); | ||
|
||
expect(testHostComponent).toBeTruthy(); | ||
}); | ||
|
||
it('should recieve the propagated valueRequiredValidator from the parent component', () => { | ||
expect(testHostComponent.timeInputComponent.valueRequiredValidator).toBe(false); | ||
}); | ||
|
||
it('should mark the form\'s validity correctly', () => { | ||
expect(testHostComponent.timeInputComponent.form.valid).toBe(true); | ||
|
||
testHostComponent.timeInputComponent.timeFormControl.setValue('2019-08-06T12:00:00Z'); | ||
|
||
testHostComponent.timeInputComponent._handleInput(); | ||
|
||
expect(testHostComponent.timeInputComponent.form.valid).toBe(false); | ||
}); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,17 @@ import { FocusMonitor } from '@angular/cdk/a11y'; | |
import { coerceBooleanProperty } from '@angular/cdk/coercion'; | ||
import { DatePipe } from '@angular/common'; | ||
import { ValueErrorStateMatcher } from '../../value-error-state-matcher'; | ||
import { Component, DoCheck, ElementRef, HostBinding, Input, OnDestroy, Optional, Self } from '@angular/core'; | ||
import { Component, DoCheck, ElementRef, HostBinding, Input, OnDestroy, OnInit, Optional, Self } from '@angular/core'; | ||
import { | ||
AbstractControl, | ||
ControlValueAccessor, | ||
FormBuilder, | ||
FormControl, | ||
FormGroup, | ||
FormGroupDirective, | ||
NgControl, | ||
NgForm, | ||
ValidatorFn, | ||
Validators | ||
} from '@angular/forms'; | ||
import { CanUpdateErrorState, CanUpdateErrorStateCtor, ErrorStateMatcher, mixinErrorState } from '@angular/material/core'; | ||
|
@@ -19,6 +21,21 @@ import { CalendarDate, CalendarPeriod, GregorianCalendarDate } from 'jdnconverti | |
import { Subject } from 'rxjs'; | ||
import { CustomRegex } from '../../custom-regex'; | ||
|
||
/** A valid time value must have both a date and a time, or both inputs must be null */ | ||
export function dateTimeValidator(otherControl: FormControl): ValidatorFn { | ||
return (control: AbstractControl): { [key: string]: any } | null => { | ||
|
||
// valid if both date and time are null or have values | ||
let invalid = !(control.value === null && otherControl.value === null || control.value !== null && otherControl.value !== null); | ||
|
||
// ensure the time value does not contain an empty string | ||
if (control.value === "" || otherControl.value === "") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't this be written all in one expression? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done in a210b88 |
||
invalid = true; | ||
} | ||
|
||
return invalid ? { 'validDateTimeRequired': { value: control.value } } : null; | ||
}; | ||
} | ||
|
||
class MatInputBase { | ||
constructor(public _defaultErrorStateMatcher: ErrorStateMatcher, | ||
|
@@ -43,7 +60,7 @@ export class DateTime { | |
styleUrls: ['./time-input.component.scss'], | ||
providers: [{ provide: MatFormFieldControl, useExisting: TimeInputComponent }] | ||
}) | ||
export class TimeInputComponent extends _MatInputMixinBase implements ControlValueAccessor, MatFormFieldControl<string>, DoCheck, CanUpdateErrorState, OnDestroy { | ||
export class TimeInputComponent extends _MatInputMixinBase implements ControlValueAccessor, MatFormFieldControl<string>, DoCheck, CanUpdateErrorState, OnDestroy, OnInit { | ||
|
||
static nextId = 0; | ||
|
||
|
@@ -59,6 +76,7 @@ export class TimeInputComponent extends _MatInputMixinBase implements ControlVal | |
|
||
@Input() dateLabel = 'Date'; | ||
@Input() timeLabel = 'Time'; | ||
@Input() valueRequiredValidator = true; | ||
|
||
dateFormControl: FormControl; | ||
timeFormControl: FormControl; | ||
|
@@ -144,6 +162,10 @@ export class TimeInputComponent extends _MatInputMixinBase implements ControlVal | |
} else { | ||
this.form.setValue({ date: null, time: null }); | ||
} | ||
|
||
this.dateFormControl.updateValueAndValidity(); | ||
this.timeFormControl.updateValueAndValidity(); | ||
|
||
this.stateChanges.next(); | ||
} | ||
|
||
|
@@ -159,9 +181,9 @@ export class TimeInputComponent extends _MatInputMixinBase implements ControlVal | |
|
||
super(_defaultErrorStateMatcher, _parentForm, _parentFormGroup, ngControl); | ||
|
||
this.dateFormControl = new FormControl(null, [Validators.required]); | ||
this.dateFormControl = new FormControl(null); | ||
|
||
this.timeFormControl = new FormControl(null, [Validators.required, Validators.pattern(CustomRegex.TIME_REGEX)]); | ||
this.timeFormControl = new FormControl(null); | ||
|
||
this.form = fb.group({ | ||
date: this.dateFormControl, | ||
|
@@ -178,6 +200,19 @@ export class TimeInputComponent extends _MatInputMixinBase implements ControlVal | |
} | ||
} | ||
|
||
ngOnInit() { | ||
if (this.valueRequiredValidator) { | ||
this.dateFormControl.setValidators([Validators.required, dateTimeValidator(this.timeFormControl)]); | ||
this.timeFormControl.setValidators([Validators.required, dateTimeValidator(this.dateFormControl), Validators.pattern(CustomRegex.TIME_REGEX)]); | ||
} else { | ||
this.dateFormControl.setValidators(dateTimeValidator(this.timeFormControl)); | ||
this.timeFormControl.setValidators([dateTimeValidator(this.dateFormControl), Validators.pattern(CustomRegex.TIME_REGEX)]); | ||
} | ||
|
||
this.dateFormControl.updateValueAndValidity(); | ||
this.timeFormControl.updateValueAndValidity(); | ||
} | ||
|
||
ngDoCheck() { | ||
if (this.ngControl) { | ||
this.updateErrorState(); | ||
|
@@ -211,6 +246,8 @@ export class TimeInputComponent extends _MatInputMixinBase implements ControlVal | |
} | ||
|
||
_handleInput(): void { | ||
this.dateFormControl.updateValueAndValidity(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this also the case in the other components like date and interval? Or is there something special about time? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nothing special about time (this time), we do it in the Date and Interval value components as well. |
||
this.timeFormControl.updateValueAndValidity(); | ||
this.onChange(this.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.
Is this change of the wrapping (datepicker directive, mat form field) intentional?
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's intentional so that the css of the calendar icon works correctly. It fixes this https://dasch.myjetbrains.com/youtrack/issue/DSP-647. Two birds, one stone 🤷
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.
Ok, great!