feat: реализовал пагинацию #86
Conversation
| <button mat-icon-button (click)="back()"> | ||
| <mat-icon>chevron_left</mat-icon> | ||
| </button> | ||
| <button mat-button *ngFor="let pageIndex of pageIndexes; let index=index" ngClass="{{activeIndex == index ? 'active' : ''}}" (click)="go(index)"> |
There was a problem hiding this comment.
- Почему использовал
ngClass, а не[class.active]=activeIndex == index? - Почему решил использовать сравнение с приведением типов (
==), а не строгое сравнение (===)? - Почему решил использовать интерполяцию?
| @@ -0,0 +1,8 @@ | |||
| export interface IPaginationContext<T> { | |||
There was a problem hiding this comment.
Я бы добавил еще appPaginationOf для того, чтобы передавать значение инпута в контексте. Поддержим функциональность используемую в стандартных структурных директивах)
| selector: '[appPagination]' | ||
| }) | ||
| export class PaginationDirective<T> implements OnChanges, OnInit{ | ||
| @Input() appPaginationOf!: T[] |
There was a problem hiding this comment.
Почему решил использовать !:? Почему не стар расширять интерфейс до | null | undefined, чтобы выходные данные были погибче и можно было работать без обертки в виде ngIf?
| } | ||
|
|
||
| ngOnInit() { | ||
| this.pageCount = this.appPaginationOf ? Math.ceil(this.appPaginationOf?.length/this.appPaginationChankSize) : 0; |
There was a problem hiding this comment.
Значение собирается из входных данных. Согласен, что они могут измениться? Может стоит реагируя на входные данные - обновлять значение на основе которых собирается контекст?
А нужен ли в this.appPaginationOf?.length optional chaining ?.?
| } | ||
|
|
||
| private getCurrentContext(currentPageIndex: number): IPaginationContext<T[]> { | ||
| const startPosition:number = this.appPaginationChankSize*currentPageIndex || 0; |
There was a problem hiding this comment.
Почему тут поставил явную типзацию, а ниже нет?
Мне видится, что типизация нигдене нужна, т.к. наследование типов для констант в данном случае работает отлично
| private getCurrentContext(currentPageIndex: number): IPaginationContext<T[]> { | ||
| const startPosition:number = this.appPaginationChankSize*currentPageIndex || 0; | ||
| const chankSize = this.appPaginationChankSize || 0; | ||
| const chankCards= this.appPaginationOf?.slice(startPosition, startPosition + chankSize); |
There was a problem hiding this comment.
Вроде appPaginationOf указан как !: T[], зачем нам тогда тут ?.?
|
|
||
| private next() { | ||
| if(this.currentPageIndex$.value + 1 < this.pageCount){ | ||
| const nextIndex = this.currentPageIndex$.value + 1; |
There was a problem hiding this comment.
Несколько раз вычисляется this.currentPageIndex$.value + 1 может стоит константу вынести до if?
| private next() { | ||
| if(this.currentPageIndex$.value + 1 < this.pageCount){ | ||
| const nextIndex = this.currentPageIndex$.value + 1; | ||
| const newIndex = nextIndex < this.appPaginationOf!.length ? nextIndex : 0; |
There was a problem hiding this comment.
nextIndex < this.appPaginationOf!.length - а валидна ли теперь данная проверка? Кажется, для данной реализации функциональность зацикливания совсем ни к чему и ее можно убрать
|
|
||
| if (shouldShowItems) { | ||
| this.currentPageIndex$.next(0); | ||
| } |
There was a problem hiding this comment.
А return не стоит ли тут добавить? Не будет ли бага при изменении входного массива, что увидим пустую страницу?
No description provided.