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

How can we propagate formGroup.reset() to every subform. #59

Closed
andreElrico opened this issue Jun 18, 2019 · 10 comments · Fixed by #110
Closed

How can we propagate formGroup.reset() to every subform. #59

andreElrico opened this issue Jun 18, 2019 · 10 comments · Fixed by #110
Assignees
Labels
effort-2: hours Will only take a few hours to fix/create flag: breaking change This feature or fix will require a breaking change flag: good for first contribution Good for a first PR on the repo scope: lib Anything related to the library itself state: community Someone from the community is working on this issue or submitted that PR state: has PR A PR is available for that issue type: feature This is a new feature

Comments

@andreElrico
Copy link

Hello all <3,

here I forked a Stackblitz of yours.

When we click on the RED reset-button. The root-Forms values are being cleared but the values in the subforms still remain.

How can we reset() the whole thing?
Should the root level form and all subforms be in total sync? two-way-bindings?
Should a new method propagateReset() be introduced? That resets itself and all sub-forms down the tree.

@andreElrico
Copy link
Author

I found some stackoverflow input here. I guess this would propagate down the tree if implemented.

However I feel like ControlValueAccessor should implement a callback for resetting();

@maxime1992
Copy link
Contributor

Hi @andreElrico 👋

Indeed this looks like a bug to me.

If nobody can pick that one up I'll take a look once I'm done with the form arrays :)

Although for now you you just create a method "reset" in the component and patch it with null values (or your default ones). Still I agree it's not convenient and should be fixed 👍

@maxime1992
Copy link
Contributor

maxime1992 commented Jun 18, 2019

I think that if you also set the values to null and patch them from the top level parent then reset would work as the default value (that is reused when you do reset!) would be null.

But yeah I'll focus in the next few days on shipping the form arrays PR as it's getting pretty huge and I'd rather focus on that one for now. Hopefully that one (reset) should be simpler I think :)

If you feel like having a go @andreElrico feel free to give it a try and raise a PR if you come to a good implementation. And even in the case where you don't manage to go through we could have a look and discuss your PR :)

Oh and I almost forgot: Thanks for all the stackblitz repro you've been doing it's really helpful!

@andreElrico
Copy link
Author

I will try to deep dive into the source and see what I can do. :)

@andreElrico
Copy link
Author

andreElrico commented Jun 19, 2019

Just some brainstorming:
Im suggesting a deliberate reset method subFormReset(options: SubFormResetOptions )

interface SubFormResetOptions {
    reset: nativeReset | initialReset ; // nativeReset: will set all values to null;
    // initialReset:  will set all values to the initial configuration.
    // (if defined as new FormControl('myInitialValue') it will be set back to 'myInitialValue' )
    // logically if there aren't any initial values both methods yield the same result.
    markAsUntouched: boolean        // if true all AbstractControls will be Untouched after subFormReset();
    markAsPristine: boolean         // if true all AbstractControls will be Pristine after subFormReset();
  • when subFormReset() is called its default configuration should behave like normal FormGroup.reset().

  • SubFormResetOptions can be provided via InjectionToken, this will allow for very flexible "form resetting"

  • Just checking for null could be problematic I guess. type="number" input will when empty write null
    --> Stackblitz please use two times backspace-key in each input field.

@maxime1992
Copy link
Contributor

Just some brainstorming

Good idea! Brainstorming is cheaper than code refactoring =)

Just checking for null could be problematic

I remember having the same thought few months ago (cf // @todo clear form? comment):

public writeValue(obj: any): void {
if (obj) {
this.formGroup.patchValue(this.transformBeforeWrite(obj), {
// required to be true otherwise it's not possible
// to be warned when the form is updated
emitEvent: true,
});
} else {
// @todo clear form?
}

And then realized it wouldn't be that easy 😅

SubFormResetOptions can be provided via InjectionToken, this will allow for very flexible "form resetting"

I think that's a good idea. We could have the default behavior provided at the highest level and then people might just override it when needed.

Just checking for null could be problematic I guess. type="number" input will when empty write null

I think it wouldn't be an issue as an input wouldn't be custom component extending one of the ngx-sub-form's classes. And for now you can only pass an object as entry. But I'd love to have support soon for primitive type values (bool, string, number) without having to put them into an object. So I'd say that it's a good idea anticipating that.

@maxime1992
Copy link
Contributor

Actually, having second thoughts.

  • It'd be nice to keep the original API where when you have a control that has a default value, it's being reset to it
  • It'd be nice to be able to support sub forms on a primitive type for later
  • It'd be nice to be able to reset a value to null, without having that value being reset to it's default

I think that one way of achieving that would be to have a provider set on the RootForm that every sub components could inject. Then once a sub component is created it'd register himself on the service and when calling reset we'd call the method from the service that'd reset the current form + sub ones.

But so far we've managed to not have anything to inject that'd have to be in the constructor and thus components extending ngx sub forms would have to inject the required services and pass them to super =(... Not sure what's the best option here.

@zakhenry any idea?

@andreElrico
Copy link
Author

I think that's a good idea. We could have the default behavior provided at the highest level and then people might just override it when needed.

I would go ng-style:

  1. register Token.
    https://github.com/angular/components/blob/8b5c0f12c823a16bdf3d4bcd767d0c42c6b0e870/src/material/form-field/form-field.ts#L93

  2. inject in every ngx-sub-form class.
    https://github.com/angular/components/blob/8b5c0f12c823a16bdf3d4bcd767d0c42c6b0e870/src/material/form-field/form-field.ts#L272

  3. If no dependency is resolved setup the "default" one.
    https://github.com/angular/components/blob/8b5c0f12c823a16bdf3d4bcd767d0c42c6b0e870/src/material/form-field/form-field.ts#L282

I dont know If I got you correctly on that, but I think we dont have to provide it on the highest level,
rather if no option is provided we fall back to a default.

I a special option is desired app wide, the user can of course root inject it in app.module.ts.

@zakhenry
Copy link
Contributor

@andreElrico @maxime1992 yep I think I agree that an injection token is probably the best option, remember that it can be managed in providers, not the constructor.

@maxime1992 maxime1992 added effort-2: hours Will only take a few hours to fix/create flag: good for first contribution Good for a first PR on the repo scope: lib Anything related to the library itself state: community Someone from the community is working on this issue or submitted that PR type: feature This is a new feature and removed help wanted labels Jun 23, 2019
@andreElrico
Copy link
Author

andreElrico commented Jun 23, 2019

  • already read through code base.
  • trying out some stuff.
  • new Idea: patching formGroup.reset() :/ ?

maxime1992 added a commit that referenced this issue Nov 6, 2019
…g those + possibility to set a formControl to null

This closes #59, #109
@maxime1992 maxime1992 added flag: breaking change This feature or fix will require a breaking change state: has PR A PR is available for that issue labels Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
effort-2: hours Will only take a few hours to fix/create flag: breaking change This feature or fix will require a breaking change flag: good for first contribution Good for a first PR on the repo scope: lib Anything related to the library itself state: community Someone from the community is working on this issue or submitted that PR state: has PR A PR is available for that issue type: feature This is a new feature
Projects
None yet
3 participants