CovidTrackerFr / vitemadose-front

Interface utilisateur de l'outil ViteMaDose
https://vitemadose.covidtracker.fr
Other
80 stars 51 forks source link

#37 paginate search results #215

Closed nhumblot closed 3 years ago

nhumblot commented 3 years ago

Fix #37

Dans vmd-rdv.view.ts, moyen fan de

this.cartesAffichees = this.infiniteScroll.ajouterCartesPaginees(this.lieuxParDepartementAffiches, this.cartesAffichees);

Assigner une référence déterminée par un appel de fonction où la référence est passée en paramètre est un code smell habituellement qui se résout par la création d'un wrapper. Sauf que si je ne change pas la référence il faut faire un appel explicite pour lancer un nouveau rendering, ce que je trouve pas spécialement meilleur :confused: . J'ai tenté de remplacer le .concat() par un .push(...) mais ça ne rafraîchit pas l'écran.

Si un reviewer a une proposition à faire sur ce sujet, elle est la bienvenue. Ça peut être un appel explicite du rendering si vous pensez que c'est mieux. :slightly_smiling_face:

nhumblot commented 3 years ago

Exemple de l'infinite scroll mis en place: infinite-scroll

fcamblor commented 3 years ago

Visible ici : https://dev.vitemado.se/37-infinite-scroll-search-results/

Pour info, il risque d'y avoir des conflits avec la branche historical-graphs (mais cette dernière devrait arriver en prod bien avant #215 donc ce sera plutôt à moi à m'aligner ;-))

bilelz commented 3 years ago

Je viens de tester, ça fonctionne bien. Quelques idées d'optimisations :

nhumblot commented 3 years ago

Je viens de tester, ça fonctionne bien. Quelques idées d'optimisations :

* ajouter un `debounce` dans la fonction this.infiniteScrollListener

* remplacer le `listener` de scroll par un `Intersection Observer`

* charger les items suivants avant que l'utilisateur arrive tout en bas de la page, 200px plus tôt par exemple

J'ai ajouté le debounce et l'offset. :+1:

Concernant l'intersection observer, cette api ne semble pas supportée par Internet Explorer. Est-ce qu'elle n'entre pas en contradiction avec https://github.com/CovidTrackerFr/vitemadose-front/issues/85 ? Ou alors ça veut dire à terme avoir un polyfill pour ie qui utilisera quand même un listener qu'on devra maintenir. :thinking:

bilelz commented 3 years ago

J'ai ajouté le debounce et l'offset. 👍

Concernant l'intersection observer, cette api ne semble pas supportée par Internet Explorer. Est-ce qu'elle n'entre pas en contradiction avec #85 ? Ou alors ça veut dire à terme avoir un polyfill pour ie qui utilisera quand même un listener qu'on devra maintenir. 🤔

Tant pis pour Internet Explorer, on lui affiche déjà une bannière pour changer de navigateur.

nhumblot commented 3 years ago

J'ai ajouté le debounce et l'offset. 👍

Concernant l'intersection observer, cette api ne semble pas supportée par Internet Explorer. Est-ce qu'elle n'entre pas en contradiction avec #85 ? Ou alors ça veut dire à terme avoir un polyfill pour ie qui utilisera quand même un listener qu'on devra maintenir. 🤔

Tant pis pour Internet Explorer, on lui affiche déjà une bannière pour changer de navigateur.

Pas de retour contradictoire donc je vais le traiter. Je vais fermer l'issue ie11. 👍

fcamblor commented 3 years ago

@nhumblot est-ce que du coup on en profiterait pas pour faire la peau à MAX_CENTER_RESULTS_COUNT (que j'avais introduit pour contourner le problème Safari) ? :-)

nhumblot commented 3 years ago

@nhumblot est-ce que du coup on en profiterait pas pour faire la peau à MAX_CENTER_RESULTS_COUNT (que j'avais introduit pour contourner le problème Safari) ? :-)

Ouep, bonne remarque. Je prends le point également. Je peux regarder demain au plus tard. 👍

nhumblot commented 3 years ago

J'ai tenu compte des commentaires de @fcamblor et @bilelz. Je vous invite à faire une relecture.

Ce qui me surprend, pour rendre l'intersection observer fonctionnel, je dois le ré-instancier pour qu'il redétecte le sentinel. Dans le cas contraire, je n'ai pas d'intersection, le sentinel n'est détecté qu'une fois. De ce que j'avais compris de la documentation, la callback devrait se déclencher à chaque fois que le sentinel est visible. Je suis preneur d'une relecture attentive et de commentaires / explications sur ce sujet.

bilelz commented 3 years ago

J'ai tenu compte des commentaires de @fcamblor et @bilelz. Je vous invite à faire une relecture.

Hello, J'ai pu regarder ce midi, ça fonctionne bien. Merci de t'être pris la tête.

Ce qui me surprend, pour rendre l'intersection observer fonctionnel, je dois le ré-instancier pour qu'il redétecte le sentinel. Dans le cas contraire, je n'ai pas d'intersection, le sentinel n'est détecté qu'une fois. De ce que j'avais compris de la documentation, la callback devrait se déclencher à chaque fois que le sentinel est visible. Je suis preneur d'une relecture attentive et de commentaires / explications sur ce sujet.

C'est sûrement parce que l'élément <div id="sentinel"></div> est redéfini dans le DOM à chaque fois que la méthode render() est exécutée.