Projet

Général

Profil

Development #57760

Couche cartographique : enlever les "propriétés" pour les mettre dans la cellule carte

Ajouté par Pierre Cros il y a plus de 2 ans. Mis à jour il y a plus de 2 ans.

Statut:
Fermé
Priorité:
Normal
Assigné à:
Version cible:
-
Début:
12 octobre 2021
Echéance:
% réalisé:

0%

Temps estimé:
Patch proposed:
Oui
Planning:
Non

Description

Pour clarifier cet écran, enlever les propriétés et les déporter dans la cellule "Carte".


Fichiers

Révisions associées

Révision 04a4ff60 (diff)
Ajouté par Valentin Deniaud il y a plus de 2 ans

maps: move properties from layer to cell (#57760)

Révision efa733b2 (diff)
Ajouté par Valentin Deniaud il y a plus de 2 ans

maps: duplicate map layer options (#57760)

Historique

#1

Mis à jour par Valentin Deniaud il y a plus de 2 ans

  • Assigné à mis à Valentin Deniaud
#2

Mis à jour par Valentin Deniaud il y a plus de 2 ans

En espérant avoir bien compris le ticket.

#3

Mis à jour par Frédéric Péters il y a plus de 2 ans

Il y a différentes choses, en fait les propriétés elles sont avant tout là pour déterminer ce qui apparaitra en popup. Donc ça serait plutôt à afficher uniquement selon la valeur posée dans marker_behaviour_onclick. Surtout ça doit quand même rester associé à la couche (on peut avoir une couche "parking voiture" et "parking vélo" avec des propriétés différentes à afficher), ça fait que ça devrait plutôt aller dans MapLayerOptions.

Aussi, c'est éventuellement à faire en rapport avec #58072, en plus des propriétés définies ici qui déterminent ce qu'il y aura dans la popup, il y a marker_colour à également inclure,

-                'properties': [x.strip() for x in l.properties.split(',')],
+                'properties': [x.strip() for x in self.properties.split(',')],

mais ça ne fera à mon avis que ramener vers #58072, parce que le js il va juste afficher tout ce qui est dispo dans la popup.

Bref tu dois inclure ici marker_colour pour ne pas casser #57296 mais ça ne résoud pas #58072.

#4

Mis à jour par Valentin Deniaud il y a plus de 2 ans

Frédéric Péters a écrit :

ça fait que ça devrait plutôt aller dans MapLayerOptions.

Yep je vais faire ça.

Aussi, c'est éventuellement à faire en rapport avec #58072, en plus des propriétés définies ici qui déterminent ce qu'il y aura dans la popup, il y a marker_colour à également inclure,

[...]

mais ça ne fera à mon avis que ramener vers #58072, parce que le js il va juste afficher tout ce qui est dispo dans la popup.

Bref tu dois inclure ici marker_colour pour ne pas casser #57296 mais ça ne résoud pas #58072.

J'ai peu envie de toucher au js ici donc j'ai envie de laisser #58072 tranquille, par contre je n'ai pas envie de casser quelque chose : et là je ne comprends pas du tout ce qui casse ici, le diff que tu as cité me laisse perplexe, je dois manquer de compréhension du code ?

#5

Mis à jour par Frédéric Péters il y a plus de 2 ans

S'il y a une option "attributs à inclure dans la popup" et qu'il y a dedans comme valeur ["titre", "description"] et que ça fait que dans le geojson il y a comme propriétés "titre" et "description", le js qui s'attend à trouver aussi la propriété définie dans marker_colour ne va pas la trouver.

Si alors tu inclus dans le geojson les propriétés "titre" et "description" + la propriété "marker_colour", l'utilisation de celle-ci ne sera pas cassée.

Mais dans la popup il y aura l'info marker_colour également, pas juste "titre" et "description". <-- le statu quo, #58072.

#6

Mis à jour par Valentin Deniaud il y a plus de 2 ans

En fait dans

-                'properties': [x.strip() for x in l.properties.split(',')],
+                'properties': [x.strip() for x in self.properties.split(',')],

je ne vois vraiment pas ce qui n'est pas équivalent puisque les propriétés sont copiées d'un objet à l'autre par la migration. J'aimerais bien comprendre ce qui « casse » pour pouvoir écrire un test de non régression.

D'ailleurs à tester cette fonctionnalité en local, je n'arrive à rien casser avec mon (nouveau) patch.

#7

Mis à jour par Frédéric Péters il y a plus de 2 ans

L'option dans la carto c'était "propriétés à inclure" (générique, pas précis), ici ça devrait être "propriétés à afficher dans la popup" (via mon commentaire "Donc ça serait plutôt à afficher uniquement selon la valeur posée dans marker_behaviour_onclick.").

Copier les données lors de la migration ça va aller, puis quelqu'un verra "couleur" dans les "propriétés à afficher dans la popup" et ira la retirer et à ce moment le rendu des marqueurs sera cassé, parce que "couleur" était la propriété précisée dans "marker colour".

On a :

  • 1/ propriétés dans le geojson
  • 2/ propriété "marker colour"
  • 3/ propriétés affichées dans la popup

Aujourd'hui on définit 1/, on pense à inclure la propriété du point 2 dedans et les propriétés du point 3, et on a tout ça qui s'affiche dans la popup 3/, qui n'est jamais définie explicitement.

Ici on va définir explicitement le point 3, mais dans le geojson il ne faudrait pas avoir exclusivement les propriétés du point 3, il faudra aussi celle du point 2.

et faire ça fera que dans la popup il y aura les propriétés de 2 et de 3 et c'est le bug et le statu quo #58072.

#8

Mis à jour par Valentin Deniaud il y a plus de 2 ans

Frédéric Péters a écrit :

L'option dans la carto c'était "propriétés à inclure" (générique, pas précis), ici ça devrait être "propriétés à afficher dans la popup"

Dac

(via mon commentaire "Donc ça serait plutôt à afficher uniquement selon la valeur posée dans marker_behaviour_onclick.").

C'était compréhensible dans la première version du patch où le champ propriétés s'affichait dans la cellule, là je trouverais ça très peu clair d'avoir le bouton d'édition de la couche geojson qui apparaît/disparaît en fonction de la valeur de marker_behaviour_onclick. Et quand bien même on ferait ça il faudra trouver une autre idée le jour où il y aura d'autres choses éditables que les propriétés. Donc autant trouver l'idée maintenant : ça pourrait le faire de désactiver le champ si marker_behaviour_onclick est « Aucun », et dans ce cas modifier le help_text pour dire d'aller changer ça avant toute chose ?

Copier les données lors de la migration ça va aller, puis quelqu'un verra "couleur" dans les "propriétés à afficher dans la popup" et ira la retirer et à ce moment le rendu des marqueurs sera cassé, parce que "couleur" était la propriété précisée dans "marker colour".

OK c'est plus clair, j'aurais eu tendance à faire l'hypothèse que #58072 pourrait être traité assez vite après pour que ce genre de malheur n'arrive pas.

#9

Mis à jour par Frédéric Péters il y a plus de 2 ans

Ah oui j'étais sur l'idée que le bouton d'édition était déjà tout le temps présente mais c'est juste là pour le paramètre opacité uniquement pour les couches "tuiles".

Donc si je comprends bien ta proposition elle est de tout le temps l'afficher, mais que dedans il y ait une note sur l'help_text du champ "propriétés pour la popup" rappelant à l'usager que le comportement actuel n'est pas d'afficher une popup. Ça m'irait de s'arrêter à ça et de laisser quand même le champ éditable.

que #58072 pourrait être traité assez vite après

oui c'était mon idée, tu auras vu maintenant un patch de Benjamin dedans, je vous laisse voir entre vous.

#10

Mis à jour par Valentin Deniaud il y a plus de 2 ans

Branche à jour avec ça.

#11

Mis à jour par Frédéric Péters il y a plus de 2 ans

  • Statut changé de Solution proposée à Solution validée
#12

Mis à jour par Valentin Deniaud il y a plus de 2 ans

  • Statut changé de Solution validée à Résolu (à déployer)
commit efa733b2cf3256f8890955cefa9f1b52d347b6ee
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Tue Nov 9 17:17:42 2021 +0100

    maps: duplicate map layer options (#57760)

commit 04a4ff60d9d63795fb1912418bd2c2af3bad28a9
Author: Valentin Deniaud <vdeniaud@entrouvert.com>
Date:   Thu Nov 4 16:32:47 2021 +0100

    maps: move properties from layer to cell (#57760)
#13

Mis à jour par Frédéric Péters il y a plus de 2 ans

  • Statut changé de Résolu (à déployer) à Solution déployée

Formats disponibles : Atom PDF