2
\$\begingroup\$

I have a small angular project, where I have an Article component to list articles with pagination system.

  1. The default page

  2. 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",
    }
}
\$\endgroup\$
  • 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ᴇᴌᴀ Dec 12 '18 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 Dec 12 '18 at 18:30
3
\$\begingroup\$

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: 20etc. After setting the default values, ngOnInitshould 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.

\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.