-
Notifications
You must be signed in to change notification settings - Fork 904
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(labs): allow validators to report customError #5310
base: main
Are you sure you want to change the base?
fix(labs): allow validators to report customError #5310
Conversation
expect(invalidListener).toHaveBeenCalledWith(jasmine.any(Event)); | ||
}); | ||
it('should report custom validation message over other validation messages', () => { | ||
const control = new TestCustomErrorConstraintValidation(); |
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 this test needs control.required = true
to ensure other validation is running
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.
Thanks @asyncLiz
I think this is covered in other tests above in the same file. Here we are just making sure that a customError
is properly reported as an error (the new test validator in the test file just does that: this.control.setCustomValidity('validator custom error');
).
Those 3 new tests fail without the fix in labs/behaviors/constraint-validation.ts. They pass after changing it to:
const customError = !!this[privateCustomValidationMessage] || validity.customError;
if (!this.control) { | ||
this.control = document.createElement('input'); | ||
} | ||
this.control.setCustomValidity('validator custom error'); |
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 needs this.control.required = state.required
to support the multi-validation test
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.
Same comment as above: we only care about customError as this is not working properly with the current codebase.
d3289ca
to
770623c
Compare
Fixes #5302
Following discussion in #5302.