feat(front): display vulnerability score#177
Conversation
front/src/components/map/popup/VulnerabilityScorePopupContent.vue
Outdated
Show resolved
Hide resolved
ludovicdmt
left a comment
There was a problem hiding this comment.
Lien carto
J'ai pas de retours sur le code (j'ai pas regardé les tests, je laisse ça à MA), ça me semble très bien ce que tu as fait dans les utils pour les futurs évolutions et la nouvelle légende + Popup.
J'ai seulement quelques trucs sur la forme, mais du au ticket qui n'était pas suffisamment détaillé sur ces points
| data-cy="switch-vulnerability-mode" | ||
| @click="switchVulnerabilityType" | ||
| > | ||
| Voir pour {{ scoreType === VulnerabilityMode.DAY ? "la nuit" : "le jour" }} |
There was a problem hiding this comment.
C'est étrange d'avoir le score de la popup qui ne correspond pas au score de la carte.
Dans les tickets, je voulais plutôt dire ne pas s'occuper de jour/nuit pour le moment (ça sera dans #173 ) mais seulement du jour.
There was a problem hiding this comment.
Oui je suis d'accord avec toi sur le fait que c'est étrange que le score de la popup ne corresponde pas à celui de la carte :/
front/src/utils/vulnerability.ts
Outdated
| export enum VulnerabilityTypeDesc { | ||
| LOW = "Basse", | ||
| MEDIUM = "Moyenne", | ||
| HIGH = "Haute" | ||
| } | ||
| export enum VulnerabilityColor { | ||
| LOW = "#006837", | ||
| MEDIUM = "#E0E0E0", | ||
| HIGH = "red", | ||
| NONE = "grey" | ||
| } |
There was a problem hiding this comment.
Et d'un point de vue plus UX, j'ai l'impression que 3 couleurs ce n'est pas suffisant.
Par rapport à la storymap.
Tu en penses quoi @QuentinMadura avec ton oeil plus entraîné que le mien
Ca peut évoluer plus tard et on peut merger en l'état je pense.
There was a problem hiding this comment.
Je suis d'accord que les trois couleurs actuelles ne suffisent pas. Un dégradé de couleurs pourrait mieux mettre en valeur les nuances entre les différentes zones. Par ailleurs, je trouve que le rouge et le vert actuels sont assez agressifs et ne valorisent pas la carte
front/src/components/map/popup/VulnerabilityScorePopupContent.vue
Outdated
Show resolved
Hide resolved
Marc-AntoineA
left a comment
There was a problem hiding this comment.
J’ai quand même terminé ma review car j’ai quelques questiions sur ton code.
Le point qui me dérange le plus c’est l’utilisation de id dans PopupData.
Pour les tests du coup on ajoute un nouveau ticket ?
| import PlantabilityLegend from "@/components/map/legend/PlantabilityLegend.vue" | ||
| import VulnerabilityLegend from "@/components/map/legend/VulnerabilityLegend.vue" | ||
|
|
||
| describe("Map legends", () => { |
There was a problem hiding this comment.
Normalement hors scope de la MR mais il manquerait le test de la légende LCZ.
| lat: 45.76, | ||
| lng: 4.85 | ||
| popupData: { | ||
| id: "0.81", |
There was a problem hiding this comment.
J’aime pas id, pour moi c’est un identifiant donc ce doit être unique.
Je regarde la suite du code encore.
Alternative : label
| <div class="flex items-center gap-2 w-full"> | ||
| <div class="w-4 h-4 rounded" :style="{ backgroundColor: getZoneColor(index) }"></div> | ||
| <span class="text-[0.9rem]" data-cy="lcz-score-popup-title">LCZ {{ index }}</span> | ||
| <div class="w-4 h-4 rounded" :style="{ backgroundColor: getZoneColor(popupData.id) }"></div> |
There was a problem hiding this comment.
Je maintiens, j’aime pas du tout id à la place de index.
| () => props.popupData.properties[`sensibilty_index_${vulnerabilityMode.value}`] | ||
| ) | ||
|
|
||
| /*const switchVulnerabilityMode = () => { |
| compact: true, | ||
| customAttribution: | ||
| '<a href="https://datagora.erasme.org/projets/calque-de-plantabilite/" target="_blank">ERASME</a>' | ||
| customAttribution: getAttributionSource() |
There was a problem hiding this comment.
si getAttributionSource() == "", est-ce qu’on veut quand même faire un new AttributionControl ?
| "7": "Ensemble dense de constructions légères", | ||
| "8": "Bâtiments bas de grande emprise", | ||
| "9": "Implantation diffuse de maisons", | ||
| const zoneDescriptions: Record<string | number, string> = { |
There was a problem hiding this comment.
Pourquoi avoir fait ça ? Ici les nombres dn’ont pas de valeurs de nombres mais simplement de label. Ça n’a par ex. pas de sens de dire "Ensemble compact de maisons = ensemble commpacts d’immeubles + ensemble compact de tours"
There was a problem hiding this comment.
Quitte à vraiment faire ça, je préfèrerais que ce soit caché derrière un ZoneIndex = string |number.
No description provided.