-
Notifications
You must be signed in to change notification settings - Fork 4
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(Interval Value Validators): Propagate valueRequiredValidator to child component (DSP-1193) #253
Conversation
…test form validity
@tobiasschweizer same thing as #251 but for the interval value component |
@@ -11,4 +11,10 @@ | |||
End is <strong>required</strong>. | |||
</mat-error> | |||
</mat-form-field> | |||
|
|||
<div class="date-form-error"> | |||
<mat-error *ngIf="((startIntervalControl.hasError('startEndSameTypeRequired') || endIntervalControl.hasError('startEndSameTypeRequired')))"> |
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 there a need for null checks for start or end as in case of the date?
Or is this taken care of by startEndSameTypeValidator
?
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 handled in startEndSameTypeValidator
return (control: AbstractControl): { [key: string]: any } | null => { | ||
|
||
let invalid = true; | ||
if (typeof(control.value) === 'number' && typeof(otherInterval.value) === 'number' || |
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 you just check for null
?
Something like invalid = !(control.value === null && otherInterval.value === null || control.value !== null && otherInterval.value !== null)
I think the input
only allows for numeric values.
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 was me being overly paranoid. Refactored in 3c0a216
@@ -157,6 +183,16 @@ export class IntervalInputComponent extends _MatInputMixinBase implements Contro | |||
} | |||
} | |||
|
|||
ngOnInit() { | |||
if (this.valueRequiredValidator) { | |||
this.startIntervalControl.setValidators([Validators.required, startEndSameTypeValidator(this.endIntervalControl)]); |
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.
Would it make sense to add the startEndSameTypeValidator
also when the required validator is not set?
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.
Definitely, I forgot. Added in 3c0a216
|
||
expect(testHostComponent.intervalInputComponent.form.valid).toBe(false); | ||
}); | ||
|
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.
Could you add a test case for the startEndSameTypeValidator
? Or is this not possible because the required operator will always trigger anyway (start or end is null
)?
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 19f996d
…ols even if the value is not required & refactors startEndSameTypeValidator validity logic
…alidator functionality
@flavens With these changes, you can make an interval not required. However you will still see an error message if the interval is not entered correctly (i.e. only the start interval has been entered). There isn't really an intuitive way to disregard this error if the interval is not required because we want to ensure the user enters a valid interval. If the user does not want to enter an interval (both inputs are empty/null), that is allowed and the form will submit. In this screenshot, the required validator is set to "false" however the interval only has a start so the user see's an error message. No interval will be submitted to Knora in this state. As for the Create Resource Form, I would recommend to scroll the user to this component if the user has filled it in only partially even if it is not required. The user should fill in the form correctly by either providing a start and an end or by leaving both inputs empty. The incorrect input will have the |
resolves DSP-1193