[Cartographie] Mode light #2539

louptheron · 2023-09-22T09:26:09Z

Commence l'utilisation de RTK's pollingInterval au lieu du composant APIWorker (deprecated)
Ajout de esbuild pour bundler serviceWorker.ts dans public/service-worker.js

Linked issues

Resolve [Cartographie] Mode hors-ligne et utilisable en navigation #2531

Tests E2E (Cypress)

ivangabriele

Il y a des trucs sympas dans cette PR :).

J'ai une seule remarque générale à faire qui va t'embêter : je crois qu'appeler ce mode "Nav" est une erreur car c'est un terme très générique utilisé pour plein de choses déjà existantes dans la terminologie web. Un peu comme "Map" d'ailleurs.

Qui plus est "Navigation" n'a pas le même sens en anglais (à part en aviation). La correspondance serait plutôt "Sailing".

Je l'aurais plutôt appelé "Light" comme cela se fait souvent sur les apps populaires. Mais on peut aussi se permettre d'utiliser des noms de code internes comme "Carbon" ou "OnBoard", du moment qu'ils sont bien documentés. Ce ne sont que des exemples, il y a l'embarras du choix ^^.

ivangabriele · 2023-09-29T07:28:00Z

...rc/main/kotlin/fr/gouv/cnsp/monitorfish/infrastructure/api/navigation/VesselNavController.kt

+    @GetMapping("/find")
+    @Operation(summary = "Get vessel information and positions")
+    fun getVessel(
+        @Parameter(description = "Vessel internal id")


Je profite de cette PR pour deux mini-propositions de goût-visibilité : est-ce que ça vous irait à @thoomasbro, @claire2212 et toi de :

sauter une ligne pour séparer visuellement les params dans les controllers et les déclaration de colonnes dans les modèles ?

essayer, dans la mesure du raisonnable et possible, d'ordonner par ordre alphabétique les props en général (à l'exception de l'ID en premier) ?

Peut-être est-ce parce que je vieillis mais ça me demande un effort de concentration notable pour m'y retrouver dans ces blocs à chaque fois 🙊.

ivangabriele · 2023-10-03T03:57:19Z

frontend/package.json

    "prepare": "cd .. && ./frontend/node_modules/.bin/husky install ./frontend/config/husky",
-    "start": "sh -ac '. ../infra/configurations/frontend/.env.local; react-scripts start'",
+    "start": "sh -ac '. ../infra/configurations/frontend/.env.local; npm run bundle-sw && react-scripts start'",


Il faudrait le mettre dans dev aussi ^^. Du coup l n'y a pas de watch dessus (via nodemon par ex.) ? Mais ça n'a d'intérêt que si on change code concerné régulièrement.

Ah yes bien vu.
Franchement ce code va être changé très très peu, je dirais que ça ne vaut pas le coup de le watch.

ivangabriele · 2023-10-03T04:01:11Z

frontend/src/api/APIWorker.tsx

@@ -133,9 +127,7 @@ export function APIWorker() {
      return
    }

-    if (vesselSidebarTab === VesselSidebarTab.VOYAGES && selectedVesselIdentity) {
-      dispatch(getVesselLogbook(selectedVesselIdentity, undefined, false))


~~Juste pour être sûr de comprendre : on ne charge plus les logs des navires (je sais que les VesselVoyage regroupent plein d'infos) ? Ou ce tab disparaît en mode "navigation" ?~~

Je viens de voir que passant en fait par RTK maintenant tu les charges directement dans le composant.

ivangabriele · 2023-10-03T04:09:43Z

frontend/src/api/index.ts

-    const token = user?.access_token
+const monitorfishBaseQuery = retry(
+  fetchBaseQuery({
+    // TODO Remove the /v1 from the baseUrl as it make harder to update APIs (vX are designed for that)


ivangabriele · 2023-10-03T04:49:29Z

frontend/src/features/Vessel/components/VesselLoader.tsx

+import { useGetVesselsLastPositionsApi } from '../hooks/useGetVesselsLastPositionsApi'
+import { showVesselsLastPosition } from '../useCases/showVesselsLastPosition'
+
+export function VesselLoader() {


Je profite de ce composant pour faire une remarque plus générale sur cette PR : il serait peut-être pas mal qu'on utilise les <Suspense /> et les dynamic components (RTK étant super pratique pour ça puisqu'on charge les données souvent directement dans le composant).

https://react.dev/reference/react/lazy#suspense-for-code-splitting

Je pense que ça peut être bien d'utiliser Suspense dans les cas où:

Un composant ou un arbre de DOM est lourd à charger

On n'est pas sûr qu'il sera chargé

Dans ce composant l'arbre est tout petit (1 div) et on est sûr qu'il sera chargé, donc je pense que ça n'est pas utile.

Par contre, OK pour l'utiliser dans d'autres endroits ou c'est plus approprié!

ivangabriele · 2023-10-03T04:55:31Z

frontend/src/hooks/authorization/useGetUserAuthorization.ts

@@ -18,7 +18,7 @@ export function useGetUserAuthorization(): UserAuthorization | undefined {
       * This is used to have backward compatibility with the Apache .htacess authentication (on `/` and `/ext`) while the authentication
       * is not yet activated, as the app is only protected by the entrypoint path.
       */
-      const isExtPage = window.location.href.includes('/ext')
+      const isExtPage = window.location.href.includes('/ext') || window.location.href.includes('/nav')


window.location.pathname === '/nav' (pareil pour /ext) ?

ivangabriele · 2023-10-03T04:58:47Z

frontend/src/workers/registerServiceWorker.ts

+      })
+
+      if (registration.installing) {
+        // eslint-disable-next-line no-console


Pour le coup, plutôt que des console logs ici, j'irai jusqu'au bout et en profiterais plutôt pour afficher des toast utilisateur qui permettent de suivre les différentes étapes.

Je ne suis pas vraiment pour perso, je ne vois pas ce que ça apporte à l'utilisateur : c'est un élément technique de l'application, à nous de "cacher" cette partie. Si on met à jour le SW, l'utilisateur n'a pas à le savoir, il doit juste aller sur l'application et que ça fonctionne

ivangabriele · 2023-10-03T05:03:05Z

frontend/src/workers/serviceWorker.ts

+self.addEventListener('fetch', event => {
+  event.respondWith(
+    (async () => {
+      const url = event.request.url.toString()


Je me demande si tout est bien pris en compte : gestion des params, suppression des hashes ?

Il y a tout ce qui est dans l'URL : https://developer.mozilla.org/en-US/docs/Web/API/Request/url
On n'a pas de hashes dans les URLs cachés

ivangabriele · 2023-10-03T05:04:16Z

frontend/src/workers/utils.ts

@@ -0,0 +1,25 @@
+import { CARTOCDN_BASEMAP, MAPBOX_BASEMAP, OPENSTREETMAP_BASEMAP, REGULATIONS, SHOM_BASEMAP } from './constants'
+
+export const getCacheKey = src => {


src => url

louptheron · 2023-10-03T07:07:41Z

Il y a des trucs sympas dans cette PR :).

J'ai une seule remarque générale à faire qui va t'embêter : je crois qu'appeler ce mode "Nav" est une erreur car c'est un terme très générique utilisé pour plein de choses déjà existantes dans la terminologie web. Un peu comme "Map" d'ailleurs.

Qui plus est "Navigation" n'a pas le même sens en anglais (à part en aviation). La correspondance serait plutôt "Sailing".

Je l'aurais plutôt appelé "Light" comme cela se fait souvent sur les apps populaires. Mais on peut aussi se permettre d'utiliser des noms de code internes comme "Carbon" ou "OnBoard", du moment qu'ils sont bien documentés. Ce ne sont que des exemples, il y a l'embarras du choix ^^.

Je te rejoins pour un terme plus adéquat comme "Light", ça me parait bien !

Par contre "Navigation" c'est bien un synonyme de "Sail" sauf que "Sailing vessel" ça veut aussi dire bateau à voile, donc p-e plus ambigu.

sonarcloud · 2023-10-03T09:23:41Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
6 Code Smells

No Coverage information
0.0% Duplication

louptheron force-pushed the loup/navigation-mode branch from d7f3c86 to e4ff86f Compare September 25, 2023 14:14

louptheron added 17 commits October 2, 2023 15:39

Refactor Healthcheck to a feature folder

742a39f

Add /nav api with les data

e9a1a76

Add /nav page and routing

b06ea70

Fix types import

f418932

Remove logbook from vessel api file

1f818e0

Add hook to get nav of bff logbook api

ef1f112

Add service worker

6c28d02

Add service worker for tiles loading

62323a2

Remove built service worker from public folder

9987eac

Refactor service worker

2f6479a

Fix sonarcloud warnings

bd79af6

Add caching of regulations. Add retry to RTK

0377065

Fix few Cypress tests. Add navigation page cypress test

3767fef

Fix cypress tests. Add README and prevent caching of regulations

5cf9e9d

Add download cypress test

57d4e6b

Use firefox and try try fix Cypress test

6e2659e

Fix env.js stub

ffb753e

louptheron force-pushed the loup/navigation-mode branch from 3d565e0 to ffb753e Compare October 2, 2023 13:39

Set OIDC as enabled in OIDC Cypress test

2774840

louptheron requested a review from ivangabriele October 2, 2023 15:25

Apply lint to backend

5a80fc3

ivangabriele approved these changes Oct 3, 2023

View reviewed changes

replace navigation by light and appy review fixes

a98cbeb

louptheron changed the title ~~[Cartographie] Mode navigation~~ [Cartographie] Mode light Oct 3, 2023

Remove Navigation term is code

be60cc6

louptheron merged commit e2a9b7f into master Oct 4, 2023
25 checks passed

louptheron deleted the loup/navigation-mode branch October 4, 2023 15:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cartographie] Mode light #2539

[Cartographie] Mode light #2539

louptheron commented Sep 22, 2023 •

edited

Loading

ivangabriele left a comment •

edited

Loading

ivangabriele Sep 29, 2023

ivangabriele Oct 3, 2023

louptheron Oct 3, 2023

ivangabriele Oct 3, 2023

ivangabriele Oct 3, 2023

ivangabriele Oct 3, 2023

louptheron Oct 3, 2023

ivangabriele Oct 3, 2023

ivangabriele Oct 3, 2023

louptheron Oct 3, 2023

ivangabriele Oct 3, 2023 •

edited

Loading

louptheron Oct 3, 2023

ivangabriele Oct 3, 2023

louptheron commented Oct 3, 2023

sonarcloud bot commented Oct 3, 2023

		@@ -0,0 +1,25 @@
		import { CARTOCDN_BASEMAP, MAPBOX_BASEMAP, OPENSTREETMAP_BASEMAP, REGULATIONS, SHOM_BASEMAP } from './constants'

		export const getCacheKey = src => {

[Cartographie] Mode light #2539

[Cartographie] Mode light #2539

Conversation

louptheron commented Sep 22, 2023 • edited Loading

Linked issues

ivangabriele left a comment • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivangabriele Oct 3, 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 commented Oct 3, 2023

sonarcloud bot commented Oct 3, 2023

louptheron commented Sep 22, 2023 •

edited

Loading

ivangabriele left a comment •

edited

Loading

ivangabriele Oct 3, 2023 •

edited

Loading