Fixed to propagate formarray validation status #263
Conversation
|
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 🙏 |
|
yes we should cover this with a e2e test. I do that at the weekend or monday. |
projects/ngx-sub-form/src/lib/deprecated/ngx-sub-form.component.ts
Outdated
Show resolved
Hide resolved
projects/ngx-sub-form/src/lib/deprecated/ngx-sub-form.component.ts
Outdated
Show resolved
Hide resolved
|
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. |
|
We should bring this into version 6.x.x. and 5.3.x. |
|
@maxime1992 could you please take a look on that? |
|
Hello, I'll take the time now. Having a look |
| controls[key] = values; | ||
| value = { | ||
| ...value, | ||
| ...values |
There was a problem hiding this comment.
I don't understand what the above is supposed to do.
values is of type MapValue[]
value is of type MapValue | undefined
So when you do ...value it'll explose the MapValue object and when you do ...values it'll explode an array of map value which is different from value.
Can you please specifiy the type on let value I think that'd help to clarify what's the intent and potentially TS would error here
There was a problem hiding this comment.
This is about to go away as well (the deprecated folder entirely).
It's been there for long enough. I know that having the fix here may be nice but I don't think it's necessary tbh so if that becomes a pain to maintain that bit of code I'd say don't bother and lets just make the fix for the latest version ;)
There was a problem hiding this comment.
I didn't spend to much investigation into the old implementation because they will be removed. This works into my fork and project. I think we should not care about this.
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 🤔 @zakhenry any idea? |
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. |
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.
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 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)? |
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.