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
fix(Time Value Validators): Propagate valueRequiredValidator to child component (DSP-1218) #257
Conversation
@tobiasschweizer FYI I formatted the input-value spec file so that's why there are 495 changes in that file. The relevant part is at the bottom. |
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 we still have this timezone issue.
@@ -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 comment
The 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 comment
The 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 comment
The 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 comment
The 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.
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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
done in a210b88
<mat-datepicker #picker></mat-datepicker> | ||
</dsp-jdn-datepicker> | ||
</mat-form-field> | ||
<dsp-jdn-datepicker [activeCalendar]="'Gregorian'"> |
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!
@@ -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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
done in a210b88
let testHostFixture: ComponentFixture<NoValueRequiredTestHostComponent>; | ||
|
||
beforeEach(async(() => { | ||
TestBed.configureTestingModule({ |
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.
Do you need this setup again?
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 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.
…o a misspelled word
resolves DSP-1218