-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
feat: support showing errors when a control is marked as touched #114
base: master
Are you sure you want to change the base?
Changes from all commits
16a5129
29867aa
f487186
c84ce6c
a9aadbf
b446c7d
baf5371
9389e60
2d6440d
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,6 +1,7 @@ | ||
import { | ||
ComponentRef, | ||
Directive, | ||
DoCheck, | ||
ElementRef, | ||
EmbeddedViewRef, | ||
Inject, | ||
|
@@ -27,6 +28,7 @@ import { | |
Observable, | ||
Subject, | ||
tap, | ||
skip, | ||
} from 'rxjs'; | ||
|
||
import { ControlErrorAnchorDirective } from './control-error-anchor.directive'; | ||
|
@@ -43,7 +45,7 @@ const errorTailorClass = 'error-tailor-has-error'; | |
'[formControlName]:not([controlErrorsIgnore]), [formControl]:not([controlErrorsIgnore]), [formGroup]:not([controlErrorsIgnore]), [formGroupName]:not([controlErrorsIgnore]), [formArrayName]:not([controlErrorsIgnore]), [ngModel]:not([controlErrorsIgnore])', | ||
exportAs: 'errorTailor', | ||
}) | ||
export class ControlErrorsDirective implements OnInit, OnDestroy { | ||
export class ControlErrorsDirective implements OnInit, OnDestroy, DoCheck { | ||
@Input('controlErrors') customErrors: ErrorsMap = {}; | ||
@Input() controlErrorsClass: string | string[] | undefined; | ||
@Input() controlCustomClass: string | string[] | undefined; | ||
|
@@ -52,6 +54,7 @@ export class ControlErrorsDirective implements OnInit, OnDestroy { | |
@Input() controlErrorsOnBlur: boolean | undefined; | ||
@Input() controlErrorsOnChange: boolean | undefined; | ||
@Input() controlErrorsOnStatusChange: boolean | undefined; | ||
@Input() controlErrorsOnTouched: boolean | undefined; | ||
@Input() controlErrorAnchor: ControlErrorAnchorDirective; | ||
|
||
private ref: ComponentRef<ControlErrorComponent>; | ||
|
@@ -62,6 +65,7 @@ export class ControlErrorsDirective implements OnInit, OnDestroy { | |
private mergedConfig: ErrorTailorConfig = {}; | ||
private customAnchorDestroyFn: () => void; | ||
private host: HTMLElement; | ||
private touchedChanges$ = new Subject<boolean>(); | ||
|
||
constructor( | ||
private vcr: ViewContainerRef, | ||
|
@@ -91,6 +95,7 @@ export class ControlErrorsDirective implements OnInit, OnDestroy { | |
let changesOnBlur$: Observable<any> = EMPTY; | ||
let changesOnChange$: Observable<any> = EMPTY; | ||
let changesOnStatusChange$: Observable<any> = EMPTY; | ||
let changesOnTouched$: Observable<boolean> = EMPTY; | ||
|
||
if (!this.controlErrorsClass || this.controlErrorsClass?.length === 0) { | ||
if (this.mergedConfig.controlErrorsClass && this.mergedConfig.controlErrorsClass) { | ||
|
@@ -118,6 +123,11 @@ export class ControlErrorsDirective implements OnInit, OnDestroy { | |
changesOnStatusChange$ = statusChanges$; | ||
} | ||
|
||
if (this.mergedConfig.controlErrorsOn.touched) { | ||
// every time the touched property changes, skipping the first emission since it's the initial state | ||
changesOnTouched$ = this.touchedChanges$.asObservable().pipe(distinctUntilChanged(), skip(1)); | ||
} | ||
|
||
if (this.isInput && this.mergedConfig.controlErrorsOn.blur) { | ||
const blur$ = fromEvent(this.host, 'focusout'); | ||
// blurFirstThenUponChange | ||
|
@@ -144,12 +154,30 @@ export class ControlErrorsDirective implements OnInit, OnDestroy { | |
.pipe(takeUntil(this.destroy)) | ||
.subscribe(() => { | ||
const hasErrors = !!this.control.errors; | ||
|
||
if (hasErrors) { | ||
this.showError(); | ||
} else { | ||
this.hideError(); | ||
} | ||
}); | ||
|
||
changesOnTouched$.pipe(takeUntil(this.destroy)).subscribe(() => { | ||
const hasErrors = !!this.control.errors; | ||
const touched = this.mergedConfig.controlErrorsOn.touched ? this.control.touched : true; | ||
|
||
if (hasErrors && touched) { | ||
this.showError(); | ||
} else { | ||
this.hideError(); | ||
} | ||
}); | ||
} | ||
|
||
ngDoCheck(): void { | ||
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. Let's use afterRender hook and only invoke it when this.mergedConfig.controlErrorsOn.touched 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. What about monkey patch ngControl methods instead? 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. I considered monkey patching since it would be the most performant solution (called the least amount of times), but I think it's not the safest solution. First of all we'd need to monkey patch all methods that might change the 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. Sure, I can look into the 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. Yes, because it'll only do it if this.mergedConfig.controlErrorsOn.touched is true 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. Sure, going to call 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. Mmm, I'm a bit puzzled. I'm pretty sure I successfully tested the
which didn't show up before… Seems a timing issue, because if I call I created a separate branch with the change, care to have a look? 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. I'm very busy at work currently. Try to comment some code and see where is the issue (You can maybe begin with setError) 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. @NetanelBasal Does it even make sense to use Furthermore, the documentation says that
so the callback would be run after every render application-wide. 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.
Correct. That's one of the main issues with this approach. I think we can go with MutationObserver or monkey patching |
||
if (this.mergedConfig.controlErrorsOn.touched) { | ||
this.touchedChanges$.next(this.control.touched); | ||
} | ||
} | ||
|
||
private setError(text: string, error?: ValidationErrors) { | ||
|
@@ -260,6 +288,7 @@ export class ControlErrorsDirective implements OnInit, OnDestroy { | |
blur: this.controlErrorsOnBlur ?? this.config.controlErrorsOn?.blur ?? true, | ||
change: this.controlErrorsOnChange ?? this.config.controlErrorsOn?.change ?? false, | ||
status: this.controlErrorsOnStatusChange ?? this.config.controlErrorsOn?.status ?? false, | ||
touched: this.controlErrorsOnTouched ?? this.config.controlErrorsOn?.touched ?? 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.
What do u think about converting the inputs to signal inputs?
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 be nice indeed! How about doing it in a separate PR, though? I believe we would also need to update the Angular version.
input
seems to be exported from v17.2.0 on, unless I'm missing something.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.
Sure