-
Notifications
You must be signed in to change notification settings - Fork 44
[1078] refactor organization-contact to reactive-form #1081
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
base: master
Are you sure you want to change the base?
[1078] refactor organization-contact to reactive-form #1081
Conversation
… 1074_add_password_confirmation
Codecov Report
@@ Coverage Diff @@
## master #1081 +/- ##
=======================================
Coverage 91.49% 91.49%
=======================================
Files 119 119
Lines 2904 2904
Branches 32 32
=======================================
Hits 2657 2657
Misses 243 243
Partials 4 4Continue to review full report at Codecov.
|
| <input type="text" class="form-control" id="email" placeholder="email" [formControl]="fg.controls.email"/> | ||
| <volontulo-form-error [fc]="fg.controls.email"></volontulo-form-error> | ||
| </div> | ||
| <label class="col-md-2 col-form-label" for="id_phone_no">Telefon</label> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You need to change for attribute to phone_no.
| private fb: FormBuilder, | ||
| private authService: AuthService, | ||
| private userService: UserService, | ||
| private organizationService: OrganizationService |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the purpose of injecting this service if you don't use it?
… 1078_refactoring_organization_contact
| class="invalid-feedback" | ||
| *ngIf="phoneNo.invalid && phoneNo.touched">Proszę podać numer telefonu.</div> | ||
| <input type="text" class="form-control" id="phone_no" placeholder="Numer telefonu" | ||
| [formControl]="fg.controls.phone_no" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use formControlName="phone_no" to register control name for fg FormGroup. Do the same way with all other controlNames.
| <input type="text" class="form-control" id="phone_no" placeholder="Numer telefonu" | ||
| [formControl]="fg.controls.phone_no" | ||
| [imask]="{mask: '+48 000-000-000'}" | ||
| [unmask]="true"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add (complete)="onComplete"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After added Travis throws error Property 'onComplete' does not exist on type 'OrganizationContactComponent'. Do You have some idea, how can I fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened separate issue #1096 for that.
juliaanholcer
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left some comments.
…organization_contact
| this.fg.controls.name.setValue(this.getFullName(user)); | ||
| this.fg.controls.email.setValue(user.email); | ||
| this.fg.controls.phone_no.setValue(user.phoneNo); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found bug there - i don't check whether is user login, or not (if not that will throw error to the console). I will fix it with an approach like in contact.component.ts
| public fg: FormGroup = this.fb.group({ | ||
| name: ['', [Validators.required, Validators.minLength(3), Validators.maxLength(30)]], | ||
| email: ['', [Validators.required, Validators.minLength(3), Validators.maxLength(30), Validators.email]], | ||
| phone_no: ['', [Validators.required, Validators.minLength(9), Validators.maxLength(9), Validators.pattern(/^[0-9]{9}$/)]], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This regex will fail for the given example. Please refactor checking phone number to a service or just accept anything here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would it fail? I made simple live example of this regex validation pattern there: https://stackblitz.com/edit/angular-xhr4qe and it seems to work fine. According to imask docs https://www.npmjs.com/package/angular-imask, unmasked value should be exactly nine digits, like in this regex.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about phone number which includes the plus sign, no-exactly 9 digits, spaces and so on?
try: +48 111-222-333
| if (this.fg.valid && !this.fg.value.honeyBunny) { | ||
| this.submitDisabled = true; | ||
| delete this.fg.value.honeyBunny; | ||
| this.contact.emit(this.fg.value as OrganizationContactPayload); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is this.contact event emitter for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't change component structure, and how it handles sending value from contact form. If I understood it correctly, form is send not from child component directly, but from parent component:
organization.component.ts:
onContact(organizationContact: OrganizationContactPayload) {
this.organization$.pipe(take(1))
.subscribe(org => this.organizationService.sendContactForm(org, organizationContact));
}
organization.component.html:
<volontulo-organization-contact *ngIf="!(isUserOrgMember$ | async)" (contact)="onContact($event)" [contactStatus]="contactStatus$ | async"> </volontulo-organization-contact>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry - I didn't notice it was moved not added 👍
ghost
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please fix phone number issue and answer my question regarding event emitter.
Description:
In organization-contact form changed from template-drive to reactive
approach