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.

Sign up for GitHub

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

Jump to bottom

[DO NOT MERGE YET] fix(lib): access class props from get form controls #113

Closed
wants to merge 2 commits into from
Closed

[DO NOT MERGE YET] fix(lib): access class props from get form controls #113

wants to merge 2 commits into from

Conversation

maxime1992
Copy link
Contributor

@maxime1992 maxime1992 commented Nov 12, 2019 •
edited
Loading

@zakhenry this PR contains a breaking change.

Even though it's technically written in the README that ngx-sub-form uses ngOnInit hook and therefore it's the consumer responsibility to call super.ngOnInit(), I don't think it'd be fair to publish that as a non breaking change because people were simply not able to call super.ngOnInit() on NgxSubFormComponent nor NgxSubFormRemapComponent so far.

For the context, please have a look on #82 where I've added some details.

@maxime1992
fix(lib): access class properties from getFormControls method
b3a93db 
BREAKING CHANGE: If you have components that are extending one of the classes of ngx-sub-form AND that have an `ngOnInit` hook, they should call `super.ngOnInit()`

This closes #82
@maxime1992
doc: fix typos
af6f127
@maxime1992 maxime1992 added scope: lib Anything related to the library itself flag: breaking change This feature or fix will require a breaking change type: bug/fix This is a bug or at least needs a fix labels Nov 12, 2019
@maxime1992 maxime1992 requested a review from zakhenry November 12, 2019 06:42
@maxime1992 maxime1992 self-assigned this Nov 12, 2019
@maxime1992
Copy link
Contributor Author

Also, I'm considering creating a next branch and target that branch for now as I'm going a bit through all the issues, in case I find another breaking change. Happy with that?

@maxime1992 maxime1992 mentioned this pull request Nov 14, 2019
Closed
@maxime1992
Copy link
Contributor Author

Just found an annoying case where I'm defining an observable based on valueChanges from a form control and as I'm not defining it within ngOnInit but directly as a property of the class, the formControl is not there yet...

@maxime1992 maxime1992 mentioned this pull request Jan 7, 2020
Closed
@maxime1992 maxime1992 changed the title fix(lib): access class props from get form controls [DO NOT MERGE YET] fix(lib): access class props from get form controls Feb 11, 2020
@maxime1992
Copy link
Contributor Author

Just found an annoying case where I'm defining an observable based on valueChanges from a form control and as I'm not defining it within ngOnInit but directly as a property of the class, the formControl is not there yet...

Discussed with @zakhenry and #86 might help as if we've got a behavior subject exposing the form group (as soon as the form is available) it wouldn't be an issue and we could simply use that obs instead

@maxime1992 maxime1992 mentioned this pull request Apr 23, 2020
Closed
@maxime1992
Copy link
Contributor Author

This is fixed in #176 in a much better way.

Closing this one as we're not going to merge it 👍

@maxime1992 maxime1992 closed this Jun 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zakhenry zakhenry zakhenry approved these changes

Assignees

@maxime1992 maxime1992

Labels
flag: breaking change This feature or fix will require a breaking change scope: lib Anything related to the library itself type: bug/fix This is a bug or at least needs a fix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@maxime1992 @zakhenry