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

feat(counters): add new map tooltips with months navigation #294

Merged

Conversation

Delapouite
Copy link
Contributor

Bonjour l'équipe.

Voici une PR ajustant les tooltips de compteurs pour se conformer à la description faite dans #275

Quelques screenshots du rendu pour commencer.

Avant cette PR:

image

Avec cette PR:

Mois "normal":

image

Mois avec record mensuel:

image

Mois avec record absolu:

image

Benoit, je suis reparti des changement récents que tu avais fait pour les tooltips des voies-lyonnaises afind d'intégrer le nouveau componentCounterTooltip.

J'ai taché de faire mon mieux avec mes maigres connaissances du framework, mais j'ai du louper un truc autour des bonnes pratiques du data-flow qui m'a conduit à devoir introduire ce JSON.parse dégueux.

Au niveau design, j'ai pas trouvé exactement les même triangles pour les flèches next/previous coté NuxtIcon.
Pour l'icone du vélo, je me suis permis de reprendre un que j'avais fait comme ça y'a meme pas à réflechir niveau copyright.
Aussi, je me suis permis de laisser une zone blanche dans le footer de la tooltip pour les mois "normaux", sinon ça fait un effet dèsagréable de tooltip sautant quand on navigue de mois en mois et que ça alterne entre présence de gélule de record ou non.

Au niveau fonctionnalités décrites par Thibaut, il manque encore le fait de basculer en mode "année", mais j'envisage de le faire dans une future PR.

En conclusion: la PR telle quelle fonctionne sans introduire de bouleversements majeurs en terme de gazillions de lignes de code changées. Benoit, n’hésite pas à mettre le dernier coup de peinture propre qui te convient par dessus et ajuster les idiomes non-nuxtiens de façon à ce que tu puisses la merger quand bon te semble suivant tes dispos.

Merci

@benoitdemaegdt
Copy link
Owner

Trop cool @Delapouite , merci pour cette PR 🙏

Benoit, je suis reparti des changement récents que tu avais fait pour les tooltips des voies-lyonnaises afind d'intégrer le nouveau componentCounterTooltip.

Parfait. Bien pratique de pouvoir utiliser un composant vue, plutôt que du HTML en template string pour faire quelque chose de réactif :)

J'ai taché de faire mon mieux avec mes maigres connaissances du framework, mais j'ai du louper un truc autour des bonnes pratiques du data-flow qui m'a conduit à devoir introduire ce JSON.parse dégueux.

Au niveau design, j'ai pas trouvé exactement les même triangles pour les flèches next/previous coté NuxtIcon.
Pour l'icone du vélo, je me suis permis de reprendre un que j'avais fait comme ça y'a meme pas à réflechir niveau copyright.
Aussi, je me suis permis de laisser une zone blanche dans le footer de la tooltip pour les mois "normaux", sinon ça fait un effet dèsagréable de tooltip sautant quand on navigue de mois en mois et que ça alterne entre présence de gélule de record ou non.

ok je regarderais ces différents points. Si ça te va je ferais éventuellement quelques commits direct sur cette PR

@benoitdemaegdt benoitdemaegdt self-requested a review March 13, 2024 06:51
@benoitdemaegdt
Copy link
Owner

J'ai taché de faire mon mieux avec mes maigres connaissances du framework, mais j'ai du louper un truc autour des bonnes pratiques du data-flow qui m'a conduit à devoir introduire ce JSON.parse dégueux.

J'ai enfin trouvé un petit moment pour regarder ça.
-> ok j'ai viré ce JSON.parse.
En fait mapbox / mapblibre n'aime pas trop qu'on lui passe des données complexes. Il y a une issue sur github, ou le maintainer dit : 'please, do not use mapbox as a database' 😂
Du coup l'idée c'est qu'au click sur la carte, on récupère juste le nom, et on va chercher la feature "source" pour avoir la data.

J'ai aussi déplacé les fonctions de calcul direct dans le composant, ça me semble plus approprié que dans le (trop) gros fichier useMap :)

je reboucle avec l'asso pour le sujet design, à voir si on laisse cet espace blanc.

@@ -138,35 +139,47 @@ onMounted(() => {
// must do this to avoid multiple popups
map.on('click', e => {
// console.log('e.lngLat >>', e.lngLat)
const features = map
Copy link
Owner

Choose a reason for hiding this comment

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

c'était assez trompeur ce nom, et ça apportait de la confusion

new maplibregl.Popup({ closeButton: false, closeOnClick: true })
.setLngLat(e.lngLat)
.setHTML(getTooltipPerspective(feature.properties))
.addTo(map);
} else if (isCompteurLayerClicked) {
const feature = features.find(({ layer }) => layer.id === 'compteurs');
const layer = layers.find(({ layer }) => layer.id === 'compteurs');
const feature = props.features.find(f => f.properties.name === layer.properties.name);
Copy link
Owner

Choose a reason for hiding this comment

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

voila c'est ici qu'on récupère la donnée source, plutot que ce que veut bien nous redonner mapbox, qui est parfois hasardeux

@benoitdemaegdt
Copy link
Owner

je propose de merger déjà sans les histoires de records.
Je commente ce code, on verra plus tard comment on remet ça.
Capture d’écran 2024-03-23 à 11 43 43

@benoitdemaegdt benoitdemaegdt merged commit fa08609 into benoitdemaegdt:main Mar 23, 2024
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.

2 participants