[Cartographie] Importer un fichier de tracé par Drag&Drop #2529

louptheron · 2023-09-19T15:15:21Z

Cette PR inirialise la sortie de monitorfishMap du composant BaseMap.
Les actions liées à la cartographie sont maintenant dans des useCases au lieu d'être dans des composants React sans rendering (approche Push plutôt que Pull).

Linked issues

Resolve Importer un fichier de tracé par Drag&Drop et le stocker dans "Mes zones importées" #1211

Tests E2E (Cypress)

ivangabriele

C'est surtout des comments de forme ;).

ivangabriele · 2023-09-20T12:54:58Z

frontend/src/features/map/BaseMap.tsx

-    initialMap.on('pointermove', event => throttleAndHandlePointerMove(event, initialMap))
-    initialMap.on('movestart', () => throttleAndHandleMovingAndZoom(initialMap))
-    initialMap.on('moveend', () => throttleAndHandleMovingAndZoom(initialMap))
+    monitorfishMap.setTarget(mapElement.current)


J'aime beaucoup ce changement de pattern pour loader OpenLayer :).

Par contre j'ai un petit soucis avec le nom. Je l'aurais plutôt tout simplement appelé openLayer, customOpenLayer ou quelque chose dans le genre (l'habitude de nommer les instances par le même nom que la classe qu'elles instancient). On n'est a bien qu'un seul non ? Par défaut on sait qu'on est sur MonitorFish.

Autre comment/question : au tout premier render, OpenLayer n'est pas encore attaché au DOM, ce n'est pas un soucis de faire sans un isFirstLoad state ?

Alors dans ce cas il faudrait plutôt l'appeler openLayerMap car cet objet est un objet de type Map, et comme je trouvais que ça n'apportait pas au métier de rajouter le terme technique openlayers j'ai mis monitorfish, mais on peut mettre tout seimplement map.

Non ce n'est pas un souci pour le rattachement au DOM, on peut le set avant, il sera render quand il est rattaché.

En effet c'est simplement le monitorfish qui me gênait mais sinon oui je te laisse choisir ^^. map me va bien (il faudra seulement faire attention dans notre vocabulaire à ce que Map / map soit bien la carte en elle-même et pas un emplacement (qu'on appelle plutôt MainWindow ou quelque chose comme ça).

ivangabriele · 2023-09-20T13:03:18Z

frontend/src/features/map/monitorfishMap.ts

C'est une lib du coup non ?

Une instance quoi, vu que c'est pas utilisé pas d'autres projets ^^

Oui j'avoue que je me demandais s'il n'avait pas sa place dans libs/ mais je te laisse choisir, les deux me paraissent logiques.

ivangabriele · 2023-09-20T13:07:32Z

frontend/src/store/reducers.ts

@@ -61,6 +62,10 @@ export const mainReducer = {
  beaconMalfunction: beaconMalfunctionReducer,
  //  TODO Pass that to singular.
  controls: controlReducer,
+  customZone: persistReducerTyped(
+    { ...getCommonPersistReducerConfig<CustomZoneState>('mainPersistorCustomZone', ['zones']) },


On doit le cloner ? Le return est une nouvelle variable normalement.

ivangabriele · 2023-09-20T13:30:20Z

frontend/src/features/CustomZone/components/CustomZone.tsx

Je ne sais pas si je suis très fan du sous-dossier générique components. J'aurais plutôt tendance à plutôt garder les composants "root" à la racine de la feature dans un sous-dossier qui porte leur nom (ici, par exemple : features/Map/CustomZone/index.tsx ou features/MapLayer/CustomZone/index.tsx. Et n'avoir comme sous-sous-dossier de features que les composants plus complexes et leurs enfants. Une feature reste dans mon esprit une "partie métier" que ce soit organisationnel (ex : Menu) ou lié à une entité (ex : Mission), on n'est pas sensé avoir de cas où le nom du composant d'une feature est le même que celui de la feature elle-même.

Dans le cas d'un menu par ex, on se retrouverait avec soit features/Menu/MainWindowMenu/index.tsx, soit avec features/MainWindow/Menu/index.tsx (je crois que tu préfères cette logique et ça me va bien ^^), mais pas avec features/Menu/Menu/index.tsx.

Le seul cas où je vois la nécessité d'un sous-dossier générique est lors de composants partagés par plusieurs composants d'une même feature (features/MyFeature/shared/MySharedComponent.tsx ou features/MyFeature/shared/MySharedComponent/index.tsx).

Pour CustomZone, c'est une liste de zones, j'ai un composant CustomZones et CustomZone, tu nommerais comment le composant enfant CustomZone si CustomZones devient index.tsx ?

Dans les patterns "feature-based" que je vois sur Internet, il ya souvent le dossier components, on peut l'enlever mais il faut qu'une feature représente bien l'élément affiché dans ce cas.

Et par exemple pour la feature Vessel, il y a VesselList, VesselSearch, VesselSidebar, etc. Il y a beaucoup de composants mais je ne vois pas de composant "maître" qui pourrait prendre le index.ts dans le dossier features/Vessel

Je me suis peut-être mal exprimé mais mon avis ne considère pas qu'il devrait y avoir un index.tsx à la racine d'une feature, au contraire qu'il ne devrait jamais y en avoir. Même lorsqu'il n'y a qu'un seul composant (pour l'instant) dans la feature (ça ne devrait pas arriver si souvent d'ailleurs).

Si on reprend ton exemple Vessel ça donne bien, par exemple :

- features/ - Vessel/ - shared/ - AFeatureLevelSharedComponent.tsx - VesselList/ - index.tsx # importe `AFeatureLevelSharedComponent.tsx` - VesselSearch/ - index.tsx # importe `AFeatureLevelSharedComponent.tsx` - VesselSidebar.tsx # pour un composant tout simple

sans index.tsx à la racine. Par contre on peut bien sûr mettre tout ce qu'on veut d'autre de commun à la racine de la feature : utils.ts, constants.ts, etc. Tout, sauf des composants. D'où l'idée du shared/ lorsqu'on en a besoin.

Et pour la feature CustomZone, j'ai l'impression que si l'on se retrouve à avoir un nom de composant égal au nom de la feature c'est :

soit que le nom de la feature n'est pas assez générique (tout en restant métier)

soit que le nom du composant de la feature n'est pas assez précis

soit que les composants de cette feature devraient aller dans une feature déjà existante

Mais dans ton cas le composant CustomZone est un enfant de CustomZones donc il devrait selon moi aller dans le dossier CustomZones, à moins qu'on l'utilise ailleurs (auquel cas on le remonte à la racine de la feature).

Et je propose de (c'est un exemple pour donner une idée) :

renommer le composant CustomZones en CustomZoneList

renommer le sous-composant CustomZone en Item ou CustomZoneItem

passer donc le composant Item dans le dossier CustomZoneList et le composant EditDialog dans le dossier CustomZoneList

- features/ - CustomZone/ - CustomZoneList/ - Item/ - EditDialog.tsx - index.tsx - index.tsx

Il me semble qu'on avait ça en tête avec @claire2212 (je te pingue pour que tu me dises si je dis des bêtises et aussi pour avoir ton avis ^^) ?

Mais peut-être que ça te gêne d'avoir de la profondeur dans les dossiers @louptheron ?

Je copie nos deux commentaires privés ici :

Loup :

Du coup ça pose la question de la hierarchie des features:
Dans ma PR précédente, j'avais Logbook : #2506
Mais en soit Logbook fait partie de la "macro" feature Vessel
Donc la question est : on met Logbook dans Vessel (mais du coup ça fait des potentiellement des slices/apis/types dans des dossiers à N-2) ?

Ivan :

Et pour répondre ici en attendant la discussion, "à chaud", ce qui m'embête avec les sous-features est que ce ne sera pas évident de différencier visuellement les sous-features des composants d'une feature. Mais je suppose que dans ce cas tu as envie d'avoir un components/ qui pour le coup a plus de sens. C'est juste qu'autant ça ne me gêne pas d'avoir du n+2 n+3 dans les les composants des features, autant le feature-based est pour moi sensé être relativement flat en ce qui concerne les features elles-mêmes (le principe feature-based ne vient pas de Facebook qui mettent tout à la racine ?).

Mais j'avoue qu'en faisant des recherches à l'instant je retrouve ce dossier components à chaque fois (vu sur 5 articles à ce sujet) donc ce que tu fais a l'air d'être la convention la plus courante 🙂
https://reboot.studio/blog/folder-structures-to-organize-react-project

Pour l'instant, je vote donc pour :

garder ton dossier components/ comme ça a l'air d'être conventionnel

pas de sous-feature pour rester flat

faire attention à avoir des naming de composants qui ne soient pas les mêmes que celui de leur feature :

des noms de feature relativement génériques tout en restant métier

des noms de composants un peu plus précis lorsqu'ils sont à la racine du composant

ne pas répêter le nom du composant parent dans un sous-component

pas de dossier shared/ :

on met les composants partagés au sein d'une même feature à la racine du sous-dossier components

composants partagés entre plusieurs features ? (on avait dit qu'on le mettrait toujours dans monitor-ui non ? j'ai un doute)

Exemple complet :

|-- components/ # ??? | |-- ProjectComponent.tsx # ??? |-- features/ | |-- Menu/ | | |-- components/ | | | |-- MainWindowRightMenu/ | | | | |-- index.tsx | | | |-- SideWindowMenu/ | | | | |-- index.tsx # importe `ProjectComponent.tsx` | |-- Vessel/ | | |-- components/ | | | |-- VesselButton.tsx | | | |-- VesselAdministrationTable/ | | | | |-- Row | | | | | |-- constants.ts | | | | | |-- Label.tsx | | | | | |-- index.tsx # importe `ProjectComponent.tsx` & `VesselButton.tsx` | | | | |-- index.tsx | | | |-- VesselListDialog/ | | | | |-- Item | | | | | |-- constants.ts | | | | | |-- index.tsx # importe `VesselButton.tsx` | | | | |-- index.tsx

ivangabriele · 2023-09-21T14:40:32Z

frontend/src/features/CustomZone/components/EditDialog.tsx

+        <Button accent={Accent.TERTIARY} onClick={onCancel}>
+          Annuler
+        </Button>
+        <Button accent={Accent.PRIMARY} disabled={!name} Icon={Icon.Save} onClick={() => name && onConfirm(name)}>


Micro détail mais j'aurais plutôt renvoyé un string | undefined et géré ça dans la fonction.

ivangabriele · 2023-09-21T14:43:00Z

frontend/src/features/CustomZone/useCases/editName.ts

+      return
+    }
+
+    const features =


features ne peut être null ou undefined ici ? Parce que je vois un .getSource()? et un forEach() ensuite, ca serait bizarre que le forEach passe en TS.

Il y a un || []

ivangabriele · 2023-09-21T14:43:46Z

frontend/src/features/CustomZone/useCases/fitToView.ts

+  const features =
+    layer
+      .getSource()
+      ?.getFeatures()


J'imagine que ça s'applique aussi ici même s'il n'y a pas de forEach pour le coup.

ivangabriele · 2023-09-21T14:44:52Z

frontend/src/features/CustomZone/useCases/remove.ts

+      return
+    }
+
+    const features =


sonarcloud · 2023-09-25T08:00:08Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

No Coverage information
0.0% Duplication

louptheron added 5 commits September 20, 2023 14:15

Add first version of custom zones

3d0faae

Fix types import

dfeb6cc

Add edit button

9ea8163

Add cypress tests

475c8ce

Update package-lock

133641d

louptheron force-pushed the loup/import-custom-zones branch from 5b8c5e9 to 133641d Compare September 20, 2023 12:16

Remove unused cypress test condition

8eb5e86

louptheron changed the title ~~[Carto] Importer un fichier de tracé par Drag&Drop~~ [Cartographie] Importer un fichier de tracé par Drag&Drop Sep 20, 2023

louptheron requested a review from ivangabriele September 20, 2023 12:21

ivangabriele approved these changes Sep 21, 2023

View reviewed changes

Few refactor afer PR review

7cc123b

louptheron merged commit 1ab9ad7 into master Sep 25, 2023
25 checks passed

louptheron deleted the loup/import-custom-zones branch September 25, 2023 10:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cartographie] Importer un fichier de tracé par Drag&Drop #2529

[Cartographie] Importer un fichier de tracé par Drag&Drop #2529

louptheron commented Sep 19, 2023 •

edited

Loading

ivangabriele left a comment

ivangabriele Sep 20, 2023

ivangabriele Sep 20, 2023

louptheron Sep 21, 2023

ivangabriele Sep 22, 2023

ivangabriele Sep 20, 2023 •

edited

Loading

louptheron Sep 21, 2023

ivangabriele Sep 22, 2023 •

edited

Loading

ivangabriele Sep 20, 2023

ivangabriele Sep 20, 2023

louptheron Sep 21, 2023 •

edited

Loading

louptheron Sep 21, 2023 •

edited

Loading

ivangabriele Sep 22, 2023 •

edited

Loading

ivangabriele Sep 22, 2023 •

edited

Loading

ivangabriele Sep 22, 2023 •

edited

Loading

ivangabriele Sep 22, 2023 •

edited

Loading

ivangabriele Sep 21, 2023

ivangabriele Sep 21, 2023

louptheron Sep 25, 2023

ivangabriele Sep 21, 2023

ivangabriele Sep 21, 2023

sonarcloud bot commented Sep 25, 2023

[Cartographie] Importer un fichier de tracé par Drag&Drop #2529

[Cartographie] Importer un fichier de tracé par Drag&Drop #2529

Conversation

louptheron commented Sep 19, 2023 • edited Loading

Linked issues

ivangabriele left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivangabriele Sep 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivangabriele Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louptheron Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

louptheron Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

ivangabriele Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

ivangabriele Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

ivangabriele Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

ivangabriele Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarcloud bot commented Sep 25, 2023

louptheron commented Sep 19, 2023 •

edited

Loading

ivangabriele Sep 20, 2023 •

edited

Loading

ivangabriele Sep 22, 2023 •

edited

Loading

louptheron Sep 21, 2023 •

edited

Loading

louptheron Sep 21, 2023 •

edited

Loading

ivangabriele Sep 22, 2023 •

edited

Loading

ivangabriele Sep 22, 2023 •

edited

Loading

ivangabriele Sep 22, 2023 •

edited

Loading

ivangabriele Sep 22, 2023 •

edited

Loading