ngx-sub-form
ngx-sub-form copied to clipboard
cloudnc
Fixed to propagate formarray validation status
I fixed the issue to propagate the formarray validation within the new implementation and also within the old implementation for backwards compatibility (so we should merge this into master and version 5.3.x). I changed the e2e test to expect the formarray validation and removed the test part for the old implementation, because the behavior is now the same. I set the locale id to DE, maybe use another locale. In german the number pipe displays the price with dots like 45.000.000. In en-US its displayed with comma like 45,000,000. But the e2e tests expects a number with dots. So to have a constant e2e run on every system we should choice a locale which prints numbers with dots or change the e2e expectation to compare against strings with commas.
I changed the emitValue property to true if the form is disabled or enabled, because the validation of sub forms is not processed if the form is enabled or disabled. So after enabling a form the validation status was not complete. Only the validation errors of the root form are available.
Sounds good, nice one. Feels like this might need an e2e test to cover what wasn't if you feel like it :pray:
yes we should cover this with a e2e test. I do that at the weekend or monday.
I think we have another issue with enable/disable. It emits the form value as often we have sub forms. So we should emit a value only its changed. Maybe a distinctUntilChanged inside the update pipe would be good. What do you think?
I created an e2e test to validate the form errors after enable and disable the form. I also fixed the issue within the old implementation.
I think we have another issue with enable/disable. It emits the form value as often we have sub forms. So we should emit a value only its changed. Maybe a distinctUntilChanged inside the update pipe would be good. What do you think?
On top of my head we have this for root forms but not sub forms. I can't remember why we didn't apply that to sub form :thinking: @zakhenry any idea?
We should bring this into version 6.x.x. and 5.3.x.
I know it'd be nice but realistically I'm not sure we need it. We've got a very limited bandwidth and energy for that project so I'm not a fan of putting more effort into the old versions while we're about to remove the deprecated API
Okay I understand. But for now, we can bring this into the master and remove then the old implementation. Why we should bring this also into 5.x.x. Because some projects we have and other people have are not on angular 14 and the library is not backward compatible.
Additionally distinctUntilChanged is not working for the enable/disable problem. Because its nessessary to emit a value for each sub form to have the right validation status of all sub forms combined. After thinking about this issues ... its not a issue and nessessary.
Any update on that? Sorry for pushing that, but this blocks a lot of projects on our side.
Any update on that? Sorry for pushing that, but this blocks a lot of projects on our side.
I think I may not have been clear enough on that. So I'll try again. I do appreciate the contribution very much but I get very limited (personal/free) time to look into this. If anybody wants to pay me to do that during my weekends so I consider it as a job rather than a library I maintain on my free time, let me know. But I do have a lot going on both at work and personally so there's no expectations to have from me. I'll contribute for free, when I can, and more importantly when I feel like it.
But this blocks a lot of projects on our side
I cannot take that into account I'm afraid. That said if it can unblock you the solution is quite simple. Make a fork of the project, rename it ngx-sub-form-anschm. Everything is setup on the repo so you should be able to get your own lib published on NPM in a matter of minutes. But I won't rush a review that seems to me like it may even contain a breaking change. I need to understand what goes in here, run a pre release on our own set of internal projects to see if our e2e tests pick up anything breaking and after that merge.
So no need to ask for an ETA every day. I do not maintain that project as part of my active workload. It's done on my free time mostly. I appreciate the contribution but if things don't move fast enough for you please just make a fork in the meantime or permanently.
Hey Maxime, I understand what you wrote in July but after 2 month I come back to this. So I spend a lot time into this issue and it feels not really good to see this waiting so long. It would be really great if you find some time to finish this. Thanks!
Hey @anschm I understand your frustration, but please be patient with us, we certainly haven't forgotten about this contribution.
In combination with us being extremely busy with our internal projects, the state of forms in angular is very much in flux with the introduction of typed forms and removal of the ability to interact with the change detection lifecycle. This has us wary of investing significant time into this project as ultimately we see this as a necessary evil, when really Angular should provide this capability out of the box.
That said, please do no let this be a blocker for you, as Maxime mentioned forking always remains an option whether it be temporary or permanent.
Hey @zakhenry, okay Angular provides now the possibility to type forms, but do you have any evidence (e.g. a ticket at angular) that Angular will provide a mechanism how to deal with re-usable forms (nested forms)?