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

Improve documentation for boxed values in form control #36746

Open
jpchavat opened this issue Apr 21, 2020 · 5 comments
Open

Improve documentation for boxed values in form control #36746

jpchavat opened this issue Apr 21, 2020 · 5 comments
Labels
area: forms cross-cutting: types freq1: low P4 A relatively minor issue that is not relevant to core functions state: confirmed type: bug/fix

Comments

@jpchavat
Copy link

I'm submitting a ...

[ ] Regression (behavior that used to work and stopped working in a new release)
[ ] Bug report 
[ ] Feature request
[x] Documentation issue or request
[ ] Support request

Current behavior ...

controlsConfig: {[key: string]: any},

and
formState: any, validatorOrOpts?: ValidatorFn|ValidatorFn[]|AbstractControlOptions|null,

The documentation about the parameter formState in the method control(..) and the parameter controlsConfig in the method group(..) (see line reference) doesn't explain very well how to set it. Additionally, the type hints has presence of anys and makes it harder to understand.

Expected behavior

I propose the follow type hints changes to clarify the situation.

For the formState type hint:
string | { value: string, disabled: boolean }

For the controlsConfig type hint:
{ [key: string]: [string | {value: string, disabled: boolean}, ...any[]] }

@treeindev
Copy link

@matsko @kapunahelewong I can send a PR with an update on the formState and controlsConfig documentation as proposed by @jpchavat on the "expected behaviour".

But I wonder if there needs to be a previous confirmation of this change or it can be done right away?

@kapunahelewong
Copy link
Contributor

Thank you, @treeindev! I'd like confirmation on this, too. Let's see if @AndrewKushnir can give us some insight on this change. @AndrewKushnir, could you share your thoughts on this?

@AndrewKushnir
Copy link
Contributor

Hi @jpchavat, thanks for creating this ticket.

It looks like there are 2 parts that we need to look into:

  • improvements to the Forms package typings. This is one of the pain points for Forms users and we plan to perform improvements in this area, see Reactive forms are not strongly typed #13721 (comment).
  • additional docs on possible values for the formState argument to better describe that an object with value and disabled keys can be used. We can look into this part once the types are updated (so we can refer to updated types in additional docs).

Thank you.

@jelbourn jelbourn changed the title Confusing parameter docs with type hints in any Improve documentation for boxed values in form control Oct 12, 2020
@jelbourn jelbourn added the P4 A relatively minor issue that is not relevant to core functions label Oct 12, 2020
@SchnWalter
Copy link
Contributor

SchnWalter commented Jul 6, 2021

I think that this "value or boxed value" argument should be completely removed, it's only adding confusion. We should just have single argument containing everything: value, disabled, validators, asyncValidators, etc. Without the ability to pass the value directly. And to offer a "simple" way to instantiate these, we could add a static method to create from value.

If new classes are added for type safety (see #38906), or even a new package, this could be easily done in the new classes and have the old ones deprecated.

@dylhunn
Copy link
Contributor

dylhunn commented Jan 28, 2022

I think that this "value or boxed value" argument should be completely removed

I agree, but unfortunately, that ship has long sailed.

However, we are going to greatly improve the documentation when typed forms lands -- ControlConfig and FormState will be pulled out into separate named typed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: forms cross-cutting: types freq1: low P4 A relatively minor issue that is not relevant to core functions state: confirmed type: bug/fix
Projects
None yet
Development

No branches or pull requests

9 participants