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

Géolocalisation et tri par distance des centres disponibles #38

Merged
merged 21 commits into from
Apr 12, 2021

Conversation

Floby
Copy link
Collaborator

@Floby Floby commented Apr 11, 2021

No description provided.

package.json Outdated Show resolved Hide resolved
src/components/vmd-appointment-card.component.ts Outdated Show resolved Hide resolved
src/views/vmd-home.view.ts Outdated Show resolved Hide resolved
src/views/vmd-rdv.view.ts Outdated Show resolved Hide resolved
src/views/vmd-rdv.view.ts Outdated Show resolved Hide resolved
src/views/vmd-rdv.view.ts Outdated Show resolved Hide resolved
src/views/vmd-rdv.view.ts Outdated Show resolved Hide resolved
@fcamblor
Copy link
Collaborator

Globalement, si tu as moyen de target dev plutôt que main pour la PR

@Floby Floby changed the base branch from main to dev April 11, 2021 15:57
src/components/vmd-appointment-card.component.ts Outdated Show resolved Hide resolved
src/views/vmd-rdv.view.ts Outdated Show resolved Hide resolved
src/views/vmd-rdv.view.ts Outdated Show resolved Hide resolved
src/views/vmd-rdv.view.ts Outdated Show resolved Hide resolved
… on radio buttons

and removed preventDefaults on click as it was preventing their default behaviour
…result was 'indisponible', then we try again as it may have been due to a timeout
…vigateur() are async,

manually triggering a re-rendering once location is resolved as lit-element cannot auto-guess this
…e times during render.

solution is to pre-calculate it everytime we fetch locations, or update user's location
…enough

I had ~50% of timeouts on my laptop with 2s timeout delay. With 4s, it was almost 90%=95%.
Copy link
Collaborator

@fcamblor fcamblor left a comment

Choose a reason for hiding this comment

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

J'ai push sur la branche geoloc un certain nombre de fixes (qui résolvent les points remontés en commentaire + d'autres plus ergonomiques)

Il y avait bel et bien plusieurs appels au lieuxDisponiblesTriés durant chaque render() => j'ai donc introduit 2 listes de lieux entre lesquelles je bascule à chaque changement de tri, afin d'éviter de re-trier.
Ces listes sont mises à jour uniquement lorsqu'on change de département.

Il y avait un problème assez pernicieux qui était dû au await sur le State.current.localisationNavigateur() : les appels en aval de cet appel étant fait de manière asynchrone, il fallait trigger manuellement un rendering (via un requestUpdate()) : lit-element ne fait pas de dirty checking lors des callbacks asynchrones, c'est pourquoi il faut l'aider à se re-render.

Malgré une augmentation du timeout du navigator.geolocation.getCurrentPosition() à 4s, je continue à avoir (rarement) des timeouts sur la récupération de la latng via desktop.
Pas sûr qu'on puisse faire beaucoup mieux.

return html`
<div class="card rounded-3 mb-5 p-4 ${classMap({clickable: this.estCliquable})}"
<div class="card rounded-3 mb-5 ${classMap({clickable: this.estCliquable})}"
style="--list-index: ${this.index}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Pour moi, ce n'est pas au composant de définir cette propriété (on pourrait très bien vouloir réutiliser le composant <vmd-appointment-card> en-dehors d'un contexte de liste où on aimerait positionner une animation dessus)

Il serait plus flexible de rajouter ce style="--list-index: ${index}" au niveau du <vmd-appointment-card> :

<vmd-appointment-card style="--list-index: ${index}" .lieu="${lieu}" .rdvPossible="${true}" .distance="${distance}" />

Et en remontant les css pour les animations au niveau du _rdv.scss :

vmd-appointment-card {
  display: block; /* <-- Nécessaire car le composant est en display: inline par défaut  */
  opacity: 1;
  animation: fade-in ease-in 200ms;
  animation-fill-mode: backwards;
  animation-delay: calc((var(--list-index) * 60ms) + 50ms);

  @keyframes fade-in {
    from {
      opacity: 0;
      filter: saturate(0%);
    }
    to {
      opacity: 1;
      filter: saturate(100%);
    }
  }
}

En bonus : une @property de moins (index) dans le composant vmd-appointment-card

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Whatever works. C'est toi l'boss

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Enfin je veux dire, j'ai essayé et ça marche pas. Mais si tu veux retenir cette PR plus longtemps, tu fais ce que tu veux :3

Copy link
Collaborator

Choose a reason for hiding this comment

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

Tu peux être un peu plus explicite que juste "ça marche pas" ?

Je veux dire, au minimum : contexte (navigateur, données), ce qui marche pas (attendu/obtenu) etc.

J'ai refait un tour sur chrome / firefox / edge / safari sous OS X et chez moi pas de soucis.

@@ -36,6 +40,48 @@ export class VmdRdvView extends LitElement {
@property({type: Array, attribute: false}) lieuxParDepartement: LieuxParDepartement | undefined = undefined;
@property({type: Boolean, attribute: false}) searchInProgress: boolean = false;

@property({type: String, attribute: false}) critèreDeTri: 'date' | 'distance' = 'date'
@property({type: Boolean, attribute: false}) geolocalisationBloquée = false
@property({type: Boolean, attribute: false}) geolocalisationIndisponible = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ce flag n'est utilisé nulle part pour un affichage à l'utilisateur

@rozierguillaume
Copy link
Contributor

Hello @fcamblor, du coup est-ce que la dernière version peut être merge ?

@rozierguillaume rozierguillaume merged commit 3532a25 into dev Apr 12, 2021
@fcamblor
Copy link
Collaborator

@rozierguillaume euh ... @Floby disait avoir des problèmes #38 (comment)

Je suis pas sûr que ce soit une bonne idée d'avoir mergée sans avoir eu son retour (vous en avez discuté en off ?)

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.

None yet

3 participants