simplifiedcourses / ngx-vest-forms

Simple form development for complex and scalable solutions
46 stars 3 forks source link

feat(validationOptions): adds support for dynamic validation options #6

Closed grimmjo closed 2 months ago

grimmjo commented 3 months ago

Hey @simplifiedcourses,

first of all thank you for this great package.

We've encountered a problem that our suite is making to many api calls as every change in the input field triggered an immediate validation as it was implemented 🙂

The idea behind this PR is to support dynamic validation options on a form, ngModelGroup and/or a ngModel. For now it is just possible to configure the debounceTime. A next step would be to move or integrate the validationConfig of the form with the validationOptions.

Thank you for your efforts and review.

simplifiedcourses commented 3 months ago

Thank you for your work! I love the idea. Some thoughts:

Brecht

simplifiedcourses commented 3 months ago

As a response to "A next step would be to move or integrate the validationConfig of the form with the validationOptions." The validationConfig is meant for the form, since that cog is responsible for triggering validations from one control to another. I'd also like to avoid breaking changes in the api unless they are really important 🚀

grimmjo commented 3 months ago

I think I found even cleaner solution.

Instead of injecting the directive everywhere, I would define validationOptions as input with a default value.

I think there is no need for looking up the options in ancestors or do I miss anything?

I will try to have look at it at the weekend and answer the other comments.

Thanks for your feedback so far.

Joachim

grimmjo commented 3 months ago

Thank you for your work! I love the idea. Some thoughts:

* I think we should back up the changes in the storybook playwright interaction tests, wdyt?

* I wrote some small remarks on the code

* Thanks a bunch! Love your contribution

Brecht

Yeah, there should be at least some tests of this feature. I haven't written any storybook playwright interaction tests so far. I try to figure it out tomorrow how they are working and some. I think the hardest part is going to detect delay.

The remarks should all be done by now. Do you resolve the conversations or should I do it?

As a response to "A next step would be to move or integrate the validationConfig of the form with the validationOptions." The validationConfig is meant for the form, since that cog is responsible for triggering validations from one control to another. I'd also like to avoid breaking changes in the api unless they are really important 🚀

I'm totally with you to avoid breaking changes. The only thing I see is the possible misunderstanding between validationConfig and validationOptions in the future. They are sounding very similiar, but doing different things. I haven't figure out yet what would be a good solution or not.

grimmjo commented 2 months ago

Hey Brecht @simplifiedcourses

any plans for a merge and a new release, yet?

Joachim

simplifiedcourses commented 2 months ago

Hey Brecht @simplifiedcourses

any plans for a merge and a new release, yet?

Joachim

Hi mate! Sorry I was on vacation. Let me check this week :-)

simplifiedcourses commented 2 months ago

:tada: This PR is included in version 1.1.0 :tada:

The release is available on:

Your semantic-release bot :package::rocket: