[Unité] Ajout des bases sur la carte #2726

ivangabriele · 2023-11-28T23:11:04Z

Linked issues

Unités – Affichage des bases sur la carte monitorenv#698

Tests E2E (Cypress)

frontend/src/domain/types/layer.ts

louptheron · 2023-11-29T10:08:31Z

frontend/src/libs/FeatureWithCodeAndEntityId.ts

+    const id = `${code}:${entityId}`
+
+    this.code = code
+    this.entityId = entityId


pourquoi ne pas rajouter ces deux propriétés dans les properties de la Feature ?

Oui en interne dans la classe pour respecter le pattern des Features OL mais si on passe par des getters pour les récupérer, sinon on en perd tout l'intérêt (garantie d'existence).

louptheron · 2023-11-29T10:10:48Z

frontend/src/utils/ensureCoordinates.ts

@@ -0,0 +1,9 @@
+import type { Coordinates } from '@mtes-mct/monitor-ui'
+
+export function ensureCoordinates(value: any): Coordinates {


Dans quel cas on n'a aucune idée du type de l'input ?

C'est simplement pour TS qui considère que c'est peut-être undefined à chaque fois qu'on utilise une méthode OL renvoyant des coordonnées, ce qui alourdit vachement le type-checking sans ce petit helper.

C'est aussi plus safe qu'un as qui ne génère pas de vrai check JS. J'hésite à faire pareil avec un ensureExtent().

louptheron · 2023-11-29T10:12:21Z

frontend/src/domain/use_cases/map/clickOnMapFeature.ts

@@ -62,6 +66,19 @@ export const clickOnMapFeature = (mapClick: MapClick) => (dispatch, getState) =>
    return
  }

+  if (mapClick.feature instanceof FeatureWithCodeAndEntityId && mapClick.feature.code === MonitorFishLayer.STATION) {
+    const overlayPosition = getDialogOverlayPositionFromFeature(mapClick.feature, 334, 320, FEATURE_MARGINS)


Ici, je préfère qu'on utilise le standard getId() comme sur les autres features

Pas de soucis mais alors je propose que l'on override la méthode getId(). L'idée et d'éviter ce genre de code à rallonge qui ne fait finalement que getter une simple propriété dont on sait pertinemment qu'elle existe.

OL a l'avatange d'être hautement extensible, je pense qu'il ne faut pas s'en priver.

Même si je pense aussi que les getter et setter en JS existent depuis un bail maintenant et getId() n'est que le prédécesseur d'ECMAScript 6 (2015) class{ get id() {} }. Mais ça c'est seulement du taste donc aucun soucis si ce n'est pas partagé.

J'ai écrit tout ce code comme une proposition généralisable.

frontend/src/domain/use_cases/map/clickOnMapFeature.ts

louptheron · 2023-11-29T10:38:09Z

frontend/src/features/Station/components/StationLayer/index.ts

+function UnmemoizedStationLayer({ hoveredFeatureId }: StationLayerProps) {
+  const vectorSourceRef = useRef(new VectorSource() as VectorSourceForFeatureWithCodeAndEntityId)
+  const vectorLayerRef = useRef(
+    new VectorLayerWithCode({


Ici aussi on pourrait utiliser le VectorLayerWithName existant ?

VectorWithName est juste un type non ? Il ne garantit pas l'existence des props.

louptheron · 2023-11-29T10:40:49Z

frontend/src/features/Station/components/StationLayer/index.ts

+
+  useEffect(() => {
+    vectorSourceRef.current.forEachFeature(feature => {
+      feature.setState({


setState n'existe pas à mon sens, enfin pas pour set les propriétés d'une feature
https://openlayers.org/en/latest/apidoc/module-ol_VectorTile-VectorTile.html#setState

Oui je l'ai nommé comme ça sans grande inspiration. Mon idée est de nommer cette méthode de manière à symboliser quelque chose qui va être muté selon les besoin une fois la feature instancié, contrairement aux props. Et que ce soit sur Env ou Fish on a un commun assez important sur le isHighlighted et le isSelected entre les layers. Je suis preneur de meilleures idées :).

louptheron · 2023-11-29T10:41:41Z

frontend/src/features/Station/components/StationLayer/index.ts

+    return () => {
+      monitorfishMap.removeLayer(vectorLayerRef.current)
+      // eslint-disable-next-line react-hooks/exhaustive-deps
+      vectorLayerRef.current.dispose()


dispose n'est requis que pour les layers en webgl ?

Là j'avoue que je colle, je te fais confiance du coup. Je veux bien une petite explication quand je reviendrai sur la logique de cleanup dans OL.

louptheron · 2023-11-29T10:46:12Z

frontend/src/features/map/types.ts

+import type VectorSource from 'ol/source/Vector'
+
+export interface VectorSourceForFeatureWithCodeAndEntityId<G extends Geometry = Geometry> extends VectorSource<G> {
+  addFeatures(features: Array<FeatureWithCodeAndEntityId<G>>): void


Je ne suis pas super fan de ces re-typages

Je suis d'accord et propose qu'on crée une classe étendue VectorSourceForFeatureWithCodeAndEntityId (ou un autre nom) étendue alors. FeatureLike garantit seulement le minimum requis pour une Feature, l'idéal serait de garantir que l'on ne manipulera que des FeatureWithCodeAndEntityId.

frontend/src/utils/getDialogOverlayPositionFromFeature.ts

ivangabriele · 2023-11-30T22:44:03Z

@louptheron Il reste :

Le highlight (coloration bleue) des bases correspondantes lorsqu'on clique sur l’icône "Centrer sur la carte" dans la dialog de liste des unités, le state Redux est déjà mis à jour et le centrage auto fonctionne.
Il restait des commentaires que tu avais fait et que j'avais remis à plus tard dans la PR des unités [Unités] Affichage et édition des unités sur la carte #2716.
Il y a un soucis avec des éléments sur la carte qui s'affichent par dessus l'overlay quand on sélectionne une base.
J'ai mis un z-index qui est sûrement bien trop élevé. Les bases étant un affichage informatif temporaire, Adeline considérait qu'elle devait prendre la prio sur tout le reste (choix qu'on a pris sur Env).

J'ai rebase la branche sur #2722 qui lui-même a été rebase sur master aujourd'hui. Je te conseille de merger cette PR-là en prio puis de rebase celle-là sur master pour simplifier le process.

sonarcloud · 2023-12-04T14:28:14Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

No Coverage information
0.0% Duplication

ivangabriele force-pushed the ivan/add-stations-on-map branch 2 times, most recently from 9cf76c8 to 131279a Compare November 29, 2023 09:54

louptheron reviewed Nov 29, 2023

View reviewed changes

ivangabriele force-pushed the ivan/add-stations-on-map branch from 131279a to b92d88f Compare November 30, 2023 19:05

ivangabriele added 6 commits December 4, 2023 10:24

Fix style issues following mini.css removal

47f64e1

Add stations on map

49fc210

Add e2e test for stations on map

7bddabd

Add unit tests to overlay positioning utils

acf4992

Extract addBufferToExtent() into utils

2e0a88a

Add unit test for displayControlUnitResourcesFromControlUnit()

0ed1e36

louptheron marked this pull request as ready for review December 4, 2023 14:22

Fix few bugs

85adf92

louptheron force-pushed the ivan/add-stations-on-map branch from 4dfc2a6 to 85adf92 Compare December 4, 2023 14:27

louptheron merged commit 0a03cf8 into master Dec 4, 2023
26 checks passed

louptheron deleted the ivan/add-stations-on-map branch December 4, 2023 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Unité] Ajout des bases sur la carte #2726

[Unité] Ajout des bases sur la carte #2726

ivangabriele commented Nov 28, 2023 •

edited

Loading

louptheron Nov 29, 2023

ivangabriele Nov 30, 2023

louptheron Nov 29, 2023

ivangabriele Nov 30, 2023 •

edited

Loading

louptheron Nov 29, 2023

ivangabriele Nov 30, 2023 •

edited

Loading

louptheron Nov 29, 2023

ivangabriele Nov 30, 2023

louptheron Nov 29, 2023

ivangabriele Nov 30, 2023

louptheron Nov 29, 2023

ivangabriele Nov 30, 2023

louptheron Nov 29, 2023

ivangabriele Nov 30, 2023 •

edited

Loading

ivangabriele commented Nov 30, 2023 •

edited

Loading

sonarcloud bot commented Dec 4, 2023

		@@ -0,0 +1,9 @@
		import type { Coordinates } from '@mtes-mct/monitor-ui'

		export function ensureCoordinates(value: any): Coordinates {

[Unité] Ajout des bases sur la carte #2726

[Unité] Ajout des bases sur la carte #2726

Conversation

ivangabriele commented Nov 28, 2023 • edited Loading

Linked issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivangabriele Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivangabriele Nov 30, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivangabriele Nov 30, 2023 • edited Loading

Choose a reason for hiding this comment

ivangabriele commented Nov 30, 2023 • edited Loading

sonarcloud bot commented Dec 4, 2023

ivangabriele commented Nov 28, 2023 •

edited

Loading

ivangabriele Nov 30, 2023 •

edited

Loading

ivangabriele Nov 30, 2023 •

edited

Loading

ivangabriele Nov 30, 2023 •

edited

Loading

ivangabriele commented Nov 30, 2023 •

edited

Loading