Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

#37 paginate search results #215

Merged
merged 15 commits into from
May 26, 2021

Conversation

nhumblot
Copy link
Collaborator

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 😕 . 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. 🙂

@nhumblot
Copy link
Collaborator Author

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

@fcamblor
Copy link
Collaborator

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
Copy link
Collaborator

bilelz commented May 18, 2021

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

@nhumblot
Copy link
Collaborator Author

nhumblot commented May 18, 2021

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. 👍

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. 🤔

@bilelz
Copy link
Collaborator

bilelz commented May 20, 2021

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.

@@ -50,7 +50,7 @@ vmd-appointment-card {
opacity: 1;
animation: fade-in ease-in 200ms;
animation-fill-mode: backwards;
animation-delay: calc((var(--list-index) * 60ms) + 50ms);
animation-delay: calc(50ms);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

c'est dommage, c'était un joli effet :)
ça doit être possible de faire la même chose en rajoutant un --page-index

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oui, malheureusement, tout a une fin.

Sauf le saucisson qui en a deux.

C'est corrigé, merci pour la précision de l'attribut. 🙂

@nhumblot
Copy link
Collaborator Author

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. 👍

src/state/InfiniteScroll.spec.ts Show resolved Hide resolved
@nhumblot nhumblot mentioned this pull request May 20, 2021
@fcamblor
Copy link
Collaborator

fcamblor commented May 21, 2021

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator Author

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.

@nhumblot nhumblot requested review from fcamblor and bilelz May 22, 2021 19:13
@bilelz
Copy link
Collaborator

bilelz commented May 25, 2021

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.

@bilelz bilelz merged commit 6543d59 into CovidTrackerFr:dev May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nombre d'affichages de centres
4 participants