feat: add pagination #92
Conversation
| @@ -0,0 +1,8 @@ | |||
| export interface IPaginationContext<T> { | |||
There was a problem hiding this comment.
Я бы добавил еще appPaginationOf для того, чтобы передавать значение инпута в контексте
| @Input() appPaginationChankSize = 6; | ||
|
|
||
| private readonly currentPage$ = new BehaviorSubject<number>(0); | ||
| private readonly pageIndexes = [1, 2, 3]; |
There was a problem hiding this comment.
А это значение где-то используется?
|
|
||
| private readonly currentPage$ = new BehaviorSubject<number>(0); | ||
| private readonly pageIndexes = [1, 2, 3]; | ||
| private readonly activeIndex = 1; |
There was a problem hiding this comment.
И это значение разве где-то используем? Не дублирует ли оно по значению currentPage$?
| private readonly currentPage$ = new BehaviorSubject<number>(0); | ||
| private readonly pageIndexes = [1, 2, 3]; | ||
| private readonly activeIndex = 1; | ||
| private pagesCount: number | undefined; |
There was a problem hiding this comment.
Может зададим initial value, таким образом избавимся от | undefined в типизации
| ) {} | ||
|
|
||
| ngOnChanges({appPaginationOf}: SimpleChanges): void { | ||
| if (appPaginationOf) { |
There was a problem hiding this comment.
А при зименении appPaginationChankSize нам не стоит выполнить updateView ?
| private getCurrentContext(currentPage: number): IPaginationContext<T> { | ||
| const startProduct = currentPage * this.appPaginationChankSize; | ||
| const endProduct = (currentPage + 1) * this.appPaginationChankSize; | ||
| const pages = this.appPaginationOf!.slice(startProduct, endProduct); |
There was a problem hiding this comment.
Я бы все эти вычисления рекомендовал вынести в отдельную чистую функцию - это поможет разгрузить метод
There was a problem hiding this comment.
чистую функцию стоит выносить из кода директивы, или можно внутри класса оставить?
There was a problem hiding this comment.
Лучше вынести из кода директивы, чтобы не засорять класс функциями, которые на нем не завязаны
|
|
||
| let pageIndexes: number[]; | ||
|
|
||
| if (currentPage === 0) { |
There was a problem hiding this comment.
Мы с тобой не обсуждали, что else/if - не лучшая практика?)
Если нет - могу подкинуть аргументов, если интересно, только дай знать🙃
There was a problem hiding this comment.
Вообще, все что ниже, стоит вынести в отдельную чистую функцию, чтобы так же разгрузить метод, назвать ее можно getPageIndexes(currentPage, this.pagesCount)
There was a problem hiding this comment.
на счет else/if, на ум приходит только switch/case. здесь он будет норм?
There was a problem hiding this comment.
Нет, я не в сторону того, что if тут не пождходит, он отлично справляется с поставленной задачей
Глобально есть 2 подхода: ранний return и if/else.
Соответсвенно у каждого подхода есть свои поклонники и противники. Соответсвенно я отношу себя к сторонникам раннего return и противником if/else. Почему? - ранний return более гибкий и читается в разы проше из-за того, что условие можно в нужный момент декомпозировать на констатны и сделать код легко читаемым, прям как книжку:
const propFirst = ...;
const propLast = ...;
if (propFirst) {
...
} else if (propLast) {
...
else {
...
}А теперь представь, что везде вместо ... n-е количество кода. Просто ли это будет читаться? Мне кажется не очень.
Теперь же другой подход:
const propLast = ...;
if (propFirst) {
...
return;
}
const propFirst = ...;
if (propLast) {
...
return;
}
...Код делает одно и то же, но в раннем ретерне все константы могут быть вычислены при необходимости, а в if/else их необходимо вычислять в самом начале. К тому же из-за явного выделения if, отступами в том числе, мы улучшаем читаемость
| @@ -38,6 +38,8 @@ export class CarouselDirective<T> implements OnChanges, OnInit { | |||
|
|
|||
| if (shouldShowItems) { | |||
| this.currentIndex$.next(0); | |||
|
|
|||
| return; | |||
There was a problem hiding this comment.
Ух ты, вот это мы на лекции пропустили, спасибо что подметил, поправлю у себя!
|
@nikolaevfo Дал ответ по комментариям твоим, посмотри, пожалуйста |
No description provided.