Joining offers form (fixes 982) #1066
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1066 +/- ##
==========================================
- Coverage 91.21% 91.18% -0.03%
==========================================
Files 116 116
Lines 2845 2847 +2
Branches 31 31
==========================================
+ Hits 2595 2596 +1
- Misses 246 247 +1
Partials 4 4
Continue to review full report at Codecov.
|
OtisRed
force-pushed
the
joining_offers
branch
from
0e8cc04 to
cf67d9c
Compare
OtisRed
changed the title
OtisRed
requested review from
a user,
TrebuhD,
juliaanholcer,
magul,
radoslawroszkowiak and
weronikadominiak
juliaanholcer
left a comment
juliaanholcer
left a comment
There was a problem hiding this comment.
In general, it looks very nice, but I have some remarks you can find in the review.
| <div class="form-group row"> | ||
| <label class="col-sm-4 col-md-3 col-lg-2 col-form-label" for="phone">Telefon:</label> | ||
| <div class="col-sm-8 col-md-9 col-lg-10"> | ||
| <input class="form-control" formControlName="phoneNo" id="phone"/> |
There was a problem hiding this comment.
You can use imask, that was provided by @stanislawK in recent PR, to display the phone number in a more friendly format and to remain consistent through the whole app.
| <input type="text" class="form-control" formControlName="honeyValue" /> | ||
| </div> | ||
| </div> | ||
| <div class="d-flex justify-content-center" style="padding: 10px"> |
There was a problem hiding this comment.
I'm not a fan of inline style, especially that we use a framework to style this app, so maybe you can use .pt-3 class to set top padding of this element.
| import {HttpClient} from '@angular/common/http'; | ||
| import {Location} from '@angular/common' | ||
|
|
||
| import {ActivatedRoute} from '@angular/router'; |
There was a problem hiding this comment.
Please, move this import up. We agreed some time ago to stick to { ImportedThing } format, so I will be very happy if you add spaces in imports after opening and before closing brackets.
| }) | ||
|
|
||
| export class OfferJoinFormComponent implements OnInit { | ||
| public error; |
There was a problem hiding this comment.
I suppose that the type of this variable is HttpRequestError or smth like this.
There was a problem hiding this comment.
I think so. I haven't put it there. IDE did it itself when I used this expresson
err => this.error = err.error.nonFieldErrors
| honeyValue: [''], | ||
| }); | ||
| public offerId: number; | ||
| public submitEnabled = false; |
There was a problem hiding this comment.
Are you using this variable somewhere? The submit button is disabled based on form validation.
There was a problem hiding this comment.
This was used in contact component as part of honey pot solution. After some fixes it works although I'm not really sure why. This variable however seems to be the glue of that solution.
| onSubmit() { | ||
| if (this.joinForm.valid) { | ||
| this.submitEnabled = true; | ||
| delete this.joinForm.value.honeyValue; |
There was a problem hiding this comment.
I'm not quite sure what is the purpose of deleting this field and how are you actually checking whether the form was filled by a bot or not.
There was a problem hiding this comment.
I kind of unreflexively applied the solution used in contact component. I guess the original idea was not to pass the honeyValue along with the rest of form values, but you're right it makes no sense in here since all is getting passed in the body is the message.
3 participants
I had some git problems in PR #1034 so I closed it and recovered it here.
This PR contains form that user uses in order to join offers.
In order to test it you have to pull also #981 as it contains endpoint this code needs to hit