I have a small angular project, where I have an Article component to list articles with pagination system.
The default page
The placeholder loader page
The main objectives are:
- Display the articles after consuming a GET API ressource.
- Paginating the articles with ngb-pagination.
- Display a placeholder loader in case the response is not ready yet.
My code works perfectly, except that I notice a little refresh of the page each time I paginate to another page. I'm sure that my code need a lot of improvements.
I'm gonna show you my code, and I ask you to help me to improve it. (you can find here on GitHub.
This my article.compononent.ts
export class ArticleComponent implements OnInit {
itemsPerPage: any;
totalItems: any;
page: any;
previousPage: any;
public observable$: Observable<any>;
private _articlesUrl = `${environment.apiUrl}` + '/articles';
public articles: any;
readonly imagePath = `${environment.apiUrl}` + '/..';
constructor(
private http: HttpClient,
) { }
ngOnInit() {
this.observable$ = this.http.get(this._articlesUrl).pipe(map((res: Response) => {
this.page = res['page'];
this.totalItems = res['total'];
this.itemsPerPage = res['limit'];
return res['_embedded']['items'];
}));
this.observable$.subscribe(
(res) => this.articles = res
);
}
loadPage(page: number) {
if (page !== this.previousPage) {
this.previousPage = page;
this.loadData();
}
}
loadData() {
this.http.get(this._articlesUrl, {
params: new HttpParams()
.set('page', this.page)
.set('per_page', this.itemsPerPage)
}).pipe(map((res) => res['_embedded']['items'])).subscribe(
(res: any[]) => this.articles = res
);
}
trackElement(index: number, element: any) {
return element ? element.id : null;
}
createRange(len= 20) {
const arr = [];
for (let i = 0; i < len ; i++) {
arr.push(i);
}
return arr;
}
}
This is the article.component.html
<div class="col-xs-12" *ngIf="articles">
<div class="blog-grids">
<div class="grid" *ngFor="let article of (articles); trackBy: trackElement">
<div class="entry-media" >
<img [src]="imagePath + article.image" alt="">
</div>
<div class="entry-body">
<span class="cat">{{article.title}}</span>
<h3><a [routerLink]="['/article/', article.id]">Visit {{article.title}}</a></h3>
<p>Nemo enim ipsam voluptatem quia voluptas sit aspernatur aut odit aut..</p>
<div class="read-more-date">
<a href="#">Read More..</a>
<span class="date">3 Hours ago</span>
</div>
</div>
</div>
</div>
<ngb-pagination [collectionSize]="totalItems" [pageSize]="itemsPerPage" [(page)]="page" [maxSize]="2" [rotate]="true" (pageChange)="loadPage($event)"></ngb-pagination>
</div>
<masonry class="row mt-5" *ngIf="!(observable$ | async)">
<masonry-brick class="col-md-4 mb-3" *ngFor="let item of createRange(6)">
<div class="timeline-item">
<div class="animated-background">
<div class="background-masker image"></div>
<div class="background-masker header-bottom"></div>
...
</div>
</div>
</masonry-brick>
</masonry>
Any code review, improvement, refactoring, recommendation, advices, pull-request would be appreciated and voted.
EDIT This is what my api return as a result:
{
"_embedded": {
"items": [
{
"categoryLabel": "category_label",
"id": 275,
"title": "Miss",
"image": "/images/articles/9f9d5128ba85f715d5bf0c72d9609b89.jpg"
},
{
"categoryLabel": "category_label",
"id": 276,
"title": "Dr.",
"image": "/images/articles/79f58611ecfe946bc41a5aba403c2c3c.jpg"
},
{
"categoryLabel": "category_label",
"id": 277,
"title": "Mr.",
"image": "/images/articles/920574ee7bcf374e2b68eb7d698a64a4.jpg"
},
{
"categoryLabel": "category_label",
"id": 278,
"title": "Prof.",
"image": "/images/articles/b5d6da63fa2db61ac4e4e8ca01568a0d.jpg"
},
{
"categoryLabel": "category_label",
"id": 279,
"title": "Prof.",
"image": "/images/articles/940d7dcbd80f8ff8f3fd707bd92f681d.jpg"
}
]
},
"route": "api_get_articles",
"parameters": [],
"absolute": false,
"limit": 5,
"total": 250,
"limitparametername": "per_page",
"page": 1,
"pages": 50,
"pageparametername": "page",
}
}
-
1\$\begingroup\$ Welcome to Code Review! After getting an answer you should not change your code anymore. This is to ensure that answers do not get invalidated and have to hit a moving target. If you have changed your code you can either post it as an answer (if it would constitute a code review) or ask a new question with your changed code (linking back to this one as reference). Refer to this post for more information \$\endgroup\$Sᴀᴍ Onᴇᴌᴀ– Sᴀᴍ Onᴇᴌᴀ ♦2018年12月12日 18:09:13 +00:00Commented Dec 12, 2018 at 18:09
-
1\$\begingroup\$ Please do not update the code in your question to incorporate feedback from answers, doing so goes against the Question + Answer style of Code Review. This is not a forum where you should keep the most updated version in your question. Please see what you may and may not do after receiving answers . I've rolled back to revision 5. \$\endgroup\$Mast– Mast ♦2018年12月12日 18:30:23 +00:00Commented Dec 12, 2018 at 18:30
1 Answer 1
The purpose of the request made in ngOnInit
seemed unclear to me.
I presume if any pagination info is not provided, then the result contains the default pagination values such as page
, total
, limit
or does it return all items?
Just in case, it is good practice to provide explicit default values such as page: 0
, limit: 20
etc. After setting the default values, ngOnInit
should use loadData
to fetch the items.
(削除) As far as i see, observable$
variable is not used anywhere except ngOnInit
, so i would remove it. (削除ここまで)
For fetching data or another possible operations on item(s) such as update, fetch, delete etc, i would create a ItemService
. You can prefer it to be inject to your component.
Since typescript is a typed language i would define types for response and item. Such as
interface Item {
}
interface ItemResponse {
page: number;
total: number;
limit: number;
items: Item[];
}
So from ItemService
you may return observable of the proper type.
Edit: updated comment about observable$
variable.
Explore related questions
See similar questions with these tags.