Skip to content

refactor: make forms reactive#26

Open
constantine0621 wants to merge 10 commits into
mainfrom
Make-forms-reactive
Open

refactor: make forms reactive#26
constantine0621 wants to merge 10 commits into
mainfrom
Make-forms-reactive

Conversation

@constantine0621

@constantine0621 constantine0621 commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Refactored the old forms in faculty, major, student, now using Reactive forms.
At the start I had a problem with ChangeDetectionStrategy.Eager not existing, fixed it by deleting and remaking tsconfig.json and node_modules.

@constantine0621
constantine0621 requested a review from a team as a code owner June 17, 2026 06:36
@constantine0621 constantine0621 changed the title Make forms reactive refactor: make forms reactive Jun 17, 2026
}

private initForm(): void{
this.majorForm = this.formBuilder.group({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think its better to use this this.formBuilder.nonNullable.group instead of majorForm!

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will add .nonNullable so that if .reset() with no arguments is ever called in the future the values won't be reset to null. majorForm! is valid because the form is initialized in ngOnInit before it is used elsewhere. I'll add the .nonNullable fix to all forms.

@constantine0621 constantine0621 self-assigned this Jun 18, 2026
DjesikaV
DjesikaV previously approved these changes Jun 22, 2026

@DjesikaV DjesikaV left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Looks good overall. ngModel is removed, the forms are using Reactive Forms now, and the add/edit dialogs open as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants