Disabled - is not included in formGroupValues

[AdditionAddict]

I noticed this.formGroup.value is being used in ngx-sub-form.component.ts rather than this.formGroup.getRawValue().

this.formGroup.value will omit any disabled controls.

Demo - https://stackblitz.com/edit/ngx-sub-form-stepper-form-demo-s5ajvo * Note how with

secondUnique: new FormControl({value:'1', disabled: true}),

that this is missing from the values.

*copy of original https://stackblitz.com/edit/ngx-sub-form-stepper-form-demo

I would be interested if this is intended behaviour? Note this hasn't come from my own use cases.

[maxime1992]

I ran into cases where I wish it was coming from getRawValue but value does reflect what formGroupe.value would return except that it's recursive. Maybe we could add a formGroupRawValues :thinking:

Any thoughts?

[zakhenry]

Yea, I think having a formGroupRawValues is the best outcome here, as it would be a breaking change otherwise and not really expected (we should be mirroring angular's behavior where it isn't broken)

[maxime1992]

Reporting back some information I discussed with @zakhenry to keep track of it there :)

Forms in general (without Angular, frameworks, libraries or even JS) have 2 things available to make a form not editable:

• disabled attribute
• readonly attribute

They have different purpose so let me clarify:

• disabled
  • Impossible to edit
  • When using the tab key to navigate with the keyboard, all disabled fields will be skipped
  • When sending the form, all the disabled fields won't be part of the data :warning:
• readonly
  • Impossible to edit
  • When using the tab key to navigate with the keyboard, you'll be able to navigate through the readonly fields
  • When sending the form, all the readonly fields will be part of the data

An interesting thread on Stackoverflow too for reference

As this is a browser behavior and not an Angular specific one, I think that by default ngx-sub-form should have the same behavior.

One possible way of dealing with that would be to provide a token and from that level, all the sub forms could return the raw value instead of the value. (if anyone has a better idea to deal with it please let me know).

BUT. I'm seriously questioning whether this is a good idea at all or not. It's just going against a standard to avoid hiding part of the form (which we want to represent as "disabled") and instead just displaying those values.

On the other hand, I can also see why this is really convenient to not rebuild a whole UI and just reuse the forms to display the values :man_shrugging:

To anyone reading this, please do share your opinion on the topic :smile_cat:

[AdditionAddict]

@maxime1992 You're completely right about the natural distinction between disabled and readonly however angular forms doesn't support readonly with this post reflecting almost exactly your thoughts above. Some of us are having to hack away using disabled in cases we ordinarily wouldn't. Hopefully with Ivy complete forms can get a little TLC...maybe.

[maxime1992]

:point_up: I've gathered some of my thoughts on the issue linked above (CF comment).

Writing things down helped me clean up my mind and I don't think that waiting for readonly attribute to be officially supported would be a good idea as this attribute doesn't even work for some form elements anyway (like select, radio, range, etc).

So, sticking with disabled is probably the only way to go for now. Why? Because behind the scene ngx-sub-form uses ControlValueAccessor and the only hook available is setDisabledState. This is our only way to "broadcast" that down to sub forms.

Soooo. From here, I think our only choice is to have a DI token set at the form level that could be read by all the sub forms.

The external api may look like:

<my-root-form
  [myData]="data"
  (update)="update($event)"
  [disabled]="shouldBeDisabled"
  [disabledType]="DisabledType.DEFAULT"
></my-root-form>

Where DisabledType could be an enum:

export enum DisabledType {
  DEFAULT = 'Default',
  RAW_VALUE = 'RawValue',
}

Internally, the token may be:

export const DISABLED_TYPE_TOKEN = new InjectionToken<Observable<DisabledType>>(
  'DisabledTypeToken'
);

But would it be weird to have an Observable as a token? :thinking: Probably not.

The reason to have an observable here would be so that any time the disabledType input changes on the root form, we can update the root form plus all the sub forms.

Without an observable (or a way to update the sub forms at least), if you're on a page looking at a resource where you should get the form value as DisabledType.DEFAULT then click on another resource, the component wouldn't get destroyed but we'd still have to make sure the form value can change to DisabledType.RAW_VALUE for example.

[zakhenry]

brainstorm sketch of how we could approach this https://www.typescriptlang.org/play/#code/MYGwhgzhAEBiD2AnAtgcUfArgB2gbwFgAoY6M6AcwFMAXaANzBEyoAoBKfU8nxWzRADsuRHmPKCkyJgDUmLAFzQA5ADN48Zd3EBfbdD0lR5ajQBKYAO5zmbToWPi+NAcIfjxklLPlUlajWUAGn0PaAATAEsIMAAjECpwm0UVWLBEYNDyQx5DYjyiYHhBCDpEK2SqaABeaABlAE9kWPgQVmVy619ldgBuYmJVTEFgGkji6EtESJoqStZVCiUEFHQsbHsBx2gikrpGEBroRYA6A5Z+ox4AeViAKypRk-CqVUjBKgAFDGwqRBoGqwDkFoJ1KiD3B4qIJMMg-nEEkpVEwIFQQtseFMZgi-McUWismRzrjTqYLF1bBx9Do+ltePwhAwmJcCkMRmMJmSKr4ADwAFQAfECmEowIIGuwlHyRPSXIyDgBtMG+AC6LK2u1KxykaxwRw+ljgOowOA4l2ImrouxoGBAlSOWNm83UqxNG3NhWKEFaVBOIHgFFY1ttlVpnpKPr9AdYXIpLCDxRtrVDYYtXsj-sDtweTwA1lQGhAE4Ik3bfOx2EA

[maxime1992]

Discussed with @zakhenry:

We should implement the solution above (to "hide" the raw value) and have a function which is recursively going through the object to search for raw values and extract them instead of the simple values. This function would be called if a given flag is set to true, for example "getRawValue".

[pogiaron]

Hi!

Thanks for the nice library. It made my work simpler. I've dealt with this problem like this:

export abstract class SubFormComponentWithDisabledControls<A> extends NgxSubFormRemapComponent<A, A> {

 protected abstract getDisabledControls(): (keyof A)[];

  writeValue(obj) {
    super.writeValue(obj);
    this.getDisabledControls().forEach(element => {
      this.formGroupControls[element].disable({emitEvent: false});
    });
  }

  protected transformToFormGroup(obj: A | null): A | null {
    return obj;
  }

  protected transformFromFormGroup(formValue: A): A | null {
    return this.formGroup.getRawValue();
  }
}

My solution may have caveats.

[danielhartig]

Using transformFromFormGroup (or fromFormGroup-Option in the new approach) to emit rawValue is definitively a possible solution. But it's also repetitive and error prone, if you need to do that in every subform in a large application. Additionally, ignoring the param in a pure function and instead using a this-reference feels like a bad workaround and should be avoided. For this reason i would prefer a more configurative solution.

[maxime1992]

For this reason i would prefer a more configurative solution.

@danielhartig have you read https://github.com/cloudnc/ngx-sub-form/issues/103#issuecomment-583022642 ? It's not like we're against a better solution it's just that we haven't found one yet. Do you have any idea/solution to offer?

[danielhartig]

I totally agree that a differentiation between disabled and readonly in Angular would be the cleanest solution. But i agree too that it is problematic to wait for it because of... reasons... A switch at root-component sounds great, if it's possible... For my needs, an additional option would be sufficient. If i could provide a Token at application/module-level, awesome!

[maxime1992]

The problem with both of these solutions are that it'll affect everything downstream and as sub forms are composable you may end up with unwanted and non default behaviors. By non default I mean that in a classic html form if a field is disabled it's not send either (99% sure but saying that from what I remember).

That's why, while not ideal... The solution to do that on a per (sub) form basis is quite nice. It's super granular and will prevent imo potential unwanted behavior.

[danielhartig]

I understand your point and i agree... from lib-owner's point of view. Nevertheless, for me as an enterprise application architect, i would like to force application-wide behaviour. With the actual implementation that's not possible. For example, now i have to do this:

public form = createForm<...>(this, {
    formType: FormType.SUB,
    formControls: {...},
    fromFormGroup: formValue => this.form.formGroup.getRawValue()
  });

Every developer have to think about this last line. I could create default-options, but if real transform-code is needed, the developer could forget this special behaviour. A better way would be a separate optionparam (e.g. disabledType as mentioned above) which i could predefine in a default-object.

const myApplicationwideDefaultOptions = {formType: FormType.SUB, disabledType: DisabledType.RAW_VALUE};
 public form = createForm<...>(this, {
    ...myApplicationwideDefaultOptions,
    formControls: {...}
  });

I think, this change would be relatively easy, flexible and has a significant benefit to my use case.

[maxime1992]

From what I understand that change doesn't need any work on ngx-sub-form side, does it?

If it's something quite easy to draft feel free to have a first stab at making a PR so we can discuss around code and see how it plays out. If not might be worth talking more about changes and impact but assuming it's an easy one, talking around code might be better

[anschm]

I created a fix for that problem. Take a look at PR https://github.com/cloudnc/ngx-sub-form/pull/265.

The solution from daniel is a good working workaround. But I think its better to have this solved within the library and have a property to control whether the raw value or the value should emitted. In this case the users of the library have the option to control that.

But in common I agree it would be better to have a distinction between readonly and disable/enable solved by angular. But actually thats not the case, maybe angular will solve this in the future.

[maxime1992]

Hi @anschm, thanks for taking a look at it. Appreciated! Unfortunately I don't think it's good enough to end up in ngx-sub-form.

That said, I think I may have an idea that would make this easier to implement properly and that could be use for other features of ngx-sub-form as well: Use the inject function which is now available from components! :)

See https://netbasal.com/unleash-the-power-of-di-functions-in-angular-2eb9f2697d66 for more info

But it could basically let us use DI in a very simple way and that could be game changing here

[anschm]

Okay, sounds good. Could you bring your idea up with some code ... it would be great to have a solution for this problem.

[maxime1992]

I really don't have any substantial amount of time to put into ngx-sub-form I'm afraid. Offering ideas and making reviews is probably the best I can do for the time being.

That said, here's my idea: define an injection token, use inject function into the sub form (and root form) to know whether we should use getRawValue or getValue. I do not know if that'd be flexible enough to have a great API but that may at least unlock a new base for that feature.