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

Fix TypedFormArray controls as array of arrays #177

Merged
merged 1 commit into from
Jun 23, 2020

Conversation

jaycetde
Copy link

controls should be an array of controls that have the single item as the value

zakhenry
zakhenry previously approved these changes Jun 19, 2020
@zakhenry zakhenry requested a review from maxime1992 June 19, 2020 20:33
Copy link
Contributor

@maxime1992 maxime1992 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zakhenry what do you think?

Good enough for now and maybe we change it to what I said along the next breaking change?

@zakhenry
Copy link
Contributor

Yea agreed, this is an improvement regardless so no reason to delay it

@maxime1992
Copy link
Contributor

@zakhenry I checked on both ngx-sub-form demo and our own app at CloudNC: We don't have any usage of TypedFormArray directly. Plus, it was supposed to be this way and therefore I assume we should consider this a simple bug fix, not a breaking change. Do you agree?

@jaycetde thanks for your first contribution and welcome 🎉

We use semantic versionning which is then used to generate changelog and releases of the library.

Could you please amend your commit with the necessary changes to taken into account my previous comment on the review and also edit the commit name to:

fix(typings): `controls` property on `TypedFormArray` 

Thanks

@zakhenry
Copy link
Contributor

Yes this is not breaking as it is correcting an incorrectly typed form. It is possible it could break someones compilation, but for the better. Their runtime would be unaffected.

@jaycetde
Copy link
Author

@maxime1992 I'll update the commit name. I'm not quite sure what you mean by "Could you please amend your commit with the necessary changes to taken into account my previous comment on the review". Are you asking me to also include the additional "preferred" changes? If so, I assume lines 24 would also need to be updated from TypedFormArray<T[K]> to TypedFormArray<T[K][0]>, correct?

controls should be an array of controls that have the single item as the value
@jaycetde jaycetde force-pushed the fix/typed-array-controls-typing branch from 15b0122 to a73d691 Compare June 22, 2020 16:55
@zakhenry
Copy link
Contributor

@jaycetde @maxime1992 I think we should limit the scope of this change to just what we have here. The commit message looks good, so the release notes will go out fine. I think this MR is good to go 👍

@zakhenry zakhenry self-requested a review June 22, 2020 16:58
@maxime1992 maxime1992 merged commit 75e2d60 into cloudnc:master Jun 23, 2020
@maxime1992
Copy link
Contributor

🎉 This PR is included in version 5.1.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@maxime1992
Copy link
Contributor

Thanks for taking the time to make this PR @jaycetde 😸!

If you feel like sharing a bit more about your usage of ngx-sub-form feel free to do so here: #112 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants