feat(paginator): add paginator component #12
Conversation
|
Pas certain de comprendre l'erreur, provideAnimations() est providé dans app.config.ts. Rien de particulier qui est différent par rapport auz autres composants |
There was a problem hiding this comment.
Semble bien fonctionner.
Seul commentaire sur le UI, lorsqu'on clique rapidement sur la flèche de gauche il a un déplacement des boutons (layout shifting) et on peut cliquer accidentellement sur un numéro de page. Je n'ai pas vérifier dans les specs de la composantes mais c'est bizarre
| nbOfElementsInList = input.required<number>(); | ||
| nbOfElementsPerPage = input.required<number>(); | ||
| middlePagesMaxRange = input<number>(1); | ||
| initialPage = input<number>(1); |
There was a problem hiding this comment.
On pourrait améliorer la lisibilité des inputs et des méthode. Au lieu, de "number of element" on pourrait utiliser "length". Pour s'inspirer Angular Material, ont souvent des noms d'input clair et mise à l'épreuve pour la compréhension externe. Par ex: le Paginator de Angular Material https://material.angular.io/components/paginator/api
| nbOfElementsInList = input.required<number>(); | |
| nbOfElementsPerPage = input.required<number>(); | |
| middlePagesMaxRange = input<number>(1); | |
| initialPage = input<number>(1); | |
| length = input.required<number>(); | |
| pageSize= input.required<number>(); | |
| middlePagesMaxRange = input<number>(1); // Vite de même je ne sais pas trop c'est quoi? | |
| pageIndex = input<number>(1); // Default to 0 ? |
There was a problem hiding this comment.
On pourrait aussi déterminer la meilleur stratégie index vs pageNumber. Je crois que l'index est plus facile a comprendre dans le cadre d'un contexte de développement.
|
Pour le problème de test tu pourrais ajouter les providers du TEST_CONFIG et dans le provider du TEST_CONFIG ajoute le "provideNoopAnimations" |
Oui en effet c'est pas idéal, mais la largeur est variable en effet. Voir ici: https://www.quebec.ca/nouvelles/rechercher Ce qui se passe dans la réalité c'est qu'en cliquant sur un numéro ou un des deux boutons ça ramènbe en haut de la page, donc le user peut pas vraiment spammer. On pourrait intéger un scrollToTop() |
| }) | ||
| export class PaginatorComponent implements OnInit { | ||
| constructor(@Inject(DOCUMENT) private document: Document) {} | ||
| injector = inject(Injector); |
There was a problem hiding this comment.
Il y a deux manière d'injecter dans Angular et tu utilises les deux dans la même composante, soit injecter dans le constructeur (ex: private document: Document) ou directement comme tu le fais. Il n'y a pas de bonne ou mauvaise mais idéallement on utilise un ou l'autre.
| this.initialPageIndexFirstChange = false; | ||
| } | ||
| }, | ||
| { injector: this.injector } |
There was a problem hiding this comment.
Angular recommande d'instancier l'effect dans le constructeur ou l'assigner en propriété. De cette manière on n'a pas besoin de l'injecteur en option.
Ex:
constructor() {
effect(() => {
const pageSize = this.pageSize();
if (!this.initialPageIndexFirstChange) {
this.currentPageIndex = 0;
this.pageChange.emit(this.currentPageIndex);
this.getNbOfPages(pageSize);
} else {
this.initialPageIndexFirstChange = false;
}
});
}
ou
private calculatePagesEffect = effect(() => {
const pageSize = this.pageSize();
if (!this.initialPageIndexFirstChange) {
this.currentPageIndex = 0;
this.pageChange.emit(this.currentPageIndex);
this.getNbOfPages(pageSize);
} else {
this.initialPageIndexFirstChange = false;
}
});
| firstPagesIndexes: number[] = []; | ||
| middlePagesIndexes: number[] = []; | ||
| lastPagesIndexes: number[] = []; | ||
| pagesIndexes: number[] = []; |
There was a problem hiding this comment.
Même ces paramètres auraient pu être convertie en computed (Signal). Il y a surement un niveau de complexité vu qu'il y a beaucoup de manipulation et dépendances. Cela pourrait être un nice to have éventuellement...
| class="page" | ||
| [class.active]="firstPageIndex === currentPageIndex" | ||
| (click)="goToPage(firstPageIndex)" | ||
| > |
There was a problem hiding this comment.
Est-ce qu'on pourrait convertir en bouton icone pour améliorer l'accessibilité?
There was a problem hiding this comment.
Ça pourrait être intéressant, mais du CSS du mat-button à ajuster pour les différents états qui je pense n'est pas idéal. J'ai modifié le html pour ajouté un keyup event et du focus.
There was a problem hiding this comment.
Ok, j'ai l'impression qu'avec le <button mat-icon-button ... tu serais arrivé au même résultat mais en utilisant les composantes de bouton.
There was a problem hiding this comment.
En fait c'est visible en faisant un hover sur les chevrons Previous et Next qui ont une ombre ciculaire au lieu de rectangulaire comme dans le SDG. Peut-être en utilisant un button régulier. Je peux tester.
There was a problem hiding this comment.
Je t'ai laissé un commentaire pour le tag <p> paragraph versus <button> que je crois pourrais être une amélioration au niveau de la sémantique et accessibilité. Mais rien de bloquant.
J'ai vérifié ce que Github faisait pour la pagination et ils utilisent des tags "anchor" qui est fait pour ce type de comportement de navigation/destination. Material aussi propose d'utiliser des
|
🎉 This PR is included in version 1.0.0-next.19 🎉 The release is available on:
Your semantic-release bot 📦🚀 |
Nouveau composant de pagination. Ajout dans la démo.