-
Notifications
You must be signed in to change notification settings - Fork 44
[986] Added component showing all offers that user joined #1077
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?
[986] Added component showing all offers that user joined #1077
Conversation
e remote-tracking branch 'upstream/master' # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit. aborts # the commit.
| <h3 class="card-header">Aktywność</h3> | ||
| <div class="card-body"> | ||
| <p class="card-text">Zgłoś się w jednej z dostępnych ofert wolontariatu i zapełnij to miejsce.</p> | ||
| <!-- <p class="card-text">Zgłoś się w jednej z dostępnych ofert wolontariatu i zapełnij to miejsce.</p> --> |
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.
Don't leave commented code behind.
|
|
||
| import { AuthService } from '../auth.service'; | ||
| import { Organization } from '../organization/organization.model'; | ||
| import { OffersBoxComponent } from '../offers/offers-box/offers-box.component'; |
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.
unused import
|
|
||
| ngOnInit() { | ||
| this.offersService.getUserOffers() | ||
| .subscribe(offers => this.offers = offers); |
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.
instead of subsribe here use async pipe in tempalate
| @Component({ | ||
| selector: 'volontulo-offers-box', | ||
| templateUrl: './offers-box.component.html', | ||
| styleUrls: ['./offers-box.component.scss'], |
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.
We do not store empty scss files.
magul
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.
One minor problem with multiple subscription to observable to be resolved.
| @@ -0,0 +1,11 @@ | |||
| <div *ngIf="(offers$ | async)?.length==0; else offersExist"> | |||
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 approach has a few flaws, the first and most obvious is being potentially exposed to multiple, unwanted, subscriptions (previously mentioned up top) that initiate requests.
See how to handle that here: https://toddmotto.com/angular-ngif-async-pipe
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 can even change order here and render list from ngIf and in else block show info, that no offers were yet joined.
| }) | ||
| export class OffersBoxComponent implements OnInit { | ||
| public offers$: Observable<Offer[]>; | ||
| constructor(private offersService: OffersService) { |
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 would add one blank line between class attributes and constructror
Also prefer (if method body is empty) to format it like that:
constructor(private offersService: OffersService) { }
|
You also have Travis build failing: https://travis-ci.org/CodeForPoznan/volontulo/builds/437120822#L1085 |
Codecov Report
@@ Coverage Diff @@
## master #1077 +/- ##
=========================================
+ Coverage 91.49% 91.5% +0.01%
=========================================
Files 119 121 +2
Lines 2904 2921 +17
Branches 32 32
=========================================
+ Hits 2657 2673 +16
- Misses 243 244 +1
Partials 4 4
Continue to review full report at Codecov.
|
For now, accessible only via '/me-working-path' endpoint.