Skip to content
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/add control errors in forms events #586

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

RaimonxDev
Copy link

Description

Hello,

FormEvents is a great utility. However, I also wanted a way to determine which controls have errors and the specific error types. To achieve this, I have added controlErrors, which returns a Record<string, string>.

I have updated the tests to ensure the expected values are correctly validated.

PS: This is my first contribution to open source! 🚀

Let me know if any changes are needed.

Copy link
Collaborator

@nartc nartc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution. I left some comments

Comment on lines 134 to 144
startWith(form),
map(() => {
if (form instanceof FormGroup) {
return errorsEvents$(form);
}
if (form instanceof AbstractControl) {
return errorsEvents$(form);
}
return null;
}),
),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: why do we startWith(form)?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I needed it to emit a value for combineLatest to ensure that I could capture an error when initializing the form. However, I realized that using startWith(null) achieves the same result.

@@ -55,6 +56,25 @@ function pristineEvents$<T>(form: AbstractControl<T>) {
);
}

function errorsEvents$(form: FormGroup | AbstractControl) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: errorsEvents$ should be Observable like other Events$ for consistency

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the feedback! I have moved the logic to errorsEvent, and it now returns an observable

formTyped$ = allEventsObservable<ReturnType<typeof this.form.getRawValue>>(
this.form,
);
$formTyped = allEventsSignal<ReturnType<typeof this.form.getRawValue>>(
this.form,
);
ngOnInit() {
this.controlData$ = allEventsObservable<string>(this.control);
Copy link
Contributor

@michael-small michael-small Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: IMO these can be lifted out of the ngOnInit into the class fields and without the injector. No need for the lifecycle hook and !/any, more declarative.

Copy link
Author

@RaimonxDev RaimonxDev Mar 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I completely agree. I just created that example to verify that it would work with the injector in the case of signals. I can fix it.

Copy link
Contributor

@michael-small michael-small left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but with one nitpick about the example app. Great addition to the util.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants