react router v7 as a framework (SPA mode) rewrite by luisdlopez · Pull Request #28 · nventive/web-react-skeleton

luisdlopez · 2025-02-06T21:26:24Z

Proposed Changes

What is the new behaviour?

Complete rewrite of the template, now using react router v7 as a framework.

It includes complete examples of:

public routes
protected routes (with auth guards at the clientLoader level)
a login form that uses clientAction method to handle submission
2 data fetching strategies, both using tanstack query, using suspense query and a combination of promises and await
localized routing

Checklist

Please check that your PR fulfills the following requirements:

Documentation has been added/updated.

Other information

Updated translations.

Seeila · 2025-02-10T19:21:44Z

frontend/Dockerfile

+COPY ./package.json package-lock.json /app/
+WORKDIR /app
+RUN npm ci --omit=dev
+


On en a encore besoin de ça?

Seeila · 2025-02-10T19:30:41Z

frontend/README.md

+Install the dependencies:
+
+```bash
+npm install


Est-ce que tu pourrais adapter le readme à Yarn et checker si le docker ne va pas nous générer un package.lock.json? Ou supprimer le ReadMe et a partie docker

Seeila · 2025-02-10T20:08:56Z

frontend/eslint.config.js

+      "react-refresh": reactRefresh.configs.vite,
+    },
+    rules: {
+      ...reactHooks.configs.recommended.rules,


Il y a un réglage ou un plugging vscode qui manque, il n'y a pas de warning quand il manque une dépendance dans les useEffect. C'est un des réglages les plus importants

Seeila · 2025-02-10T20:16:35Z

frontend/src/app.css

+
+html,
+body {
+  min-height: 100vh;


Peux-tu utiliser du dvh à la place? Ça règle les problèmes des browser qui ne prennent pas en compte quand le clavier est visible sur mobile

Seeila · 2025-02-10T20:36:17Z

frontend/src/root.tsx

+  Scripts,
+  ScrollRestoration,
+} from "react-router";
+import type { Route } from "./+types/root";


J'ai une erreur ici avec le path, est-ce qu'il y a des réglages manquants pour que ça ne soit pas le cas

Seeila · 2025-02-10T20:37:59Z

frontend/src/root.tsx

+  );
+}
+
+export function ErrorBoundary({ error }: Route.ErrorBoundaryProps) {


Ça gagnerait à être dans son propre component, je pense

Seeila · 2025-02-10T20:52:10Z

frontend/src/app/components/cookieConsent/cookieBanner/cookie-banner.module.css

 .cookie-banner {
  width: 100%;
  position: fixed;
+  /* stylelint-disable-next-line custom-property-pattern */


Pas moyen de désactiver le réglage dans eslint au lieu de plouer les css avec ça?

Seeila · 2025-02-10T20:58:10Z

frontend/src/app/components/cookieConsent/cookieModal/CookieModal.tsx

          <Link
-            className="mr-a"
-            href={t("cookie_consent_link")}
+            sx={(theme) => ({ marginRight: theme.customProperties.spacing.a })}


C'est pas bon d'utiliser le spacing de mui par multiple de 8 avec theme.spacing(x)?
Je suis encore traumatisée par toutes les variables de Bootstrap où je retenais pas qu'est-ce qui valait quoi, je devais toujours avoir le fichier des variables ouvert :')

VincentErb · 2025-02-12T15:09:30Z

frontend/docs/routing.md

+
+Note that the path to the locales in these files is different (`#assets` instead of the expcted `@assets`).  
+When using react router as a framework, the routes are built via a vite.js plugin, running in node.js (not in the browser).  
+The alias here is different than the one define in the tsconfig file. This alias comes from the `imports` property in the package.json file.


Suggested change

The alias here is different than the one define in the tsconfig file. This alias comes from the `imports` property in the package.json file.

The alias here is different than the one defined in the tsconfig file. This alias comes from the `imports` property in the package.json file.

VincentErb · 2025-02-12T15:10:16Z

frontend/docs/data-fetching-strategies.md

+**`On subsequent renders`**, the `useSuspenseQuery` hook will return stale data while fetching any new data.
+
+- **Pros**: The `useSuspenseQuery` hook ensures the component only renders when data is available.
+- **Pros**: The `useSuspenseQuery` returns stale data immediately (if available) while new data is fetched.


Est-ce que c'est vraiment un pro dans toutes les situations ? Est-ce que ce comportement est configurable ?

VincentErb · 2025-02-12T15:11:44Z

frontend/docs/routing.md

+
+This ensures that the dashboard route is only accessible to authenticated users.
+
+At the moment, react rouvet v7 does not support middlewares (it is planned though). You have a couple of options as to **where** to use the `clientLoaderAuthGuard`:


Suggested change

At the moment, react rouvet v7 does not support middlewares (it is planned though). You have a couple of options as to **where** to use the `clientLoaderAuthGuard`:

At the moment, react router v7 does not support middlewares (it is planned though). You have a couple of options as to **where** to use the `clientLoaderAuthGuard`:

VincentErb · 2025-02-12T15:13:49Z

frontend/sheet2i18n.config.cjs

    "https://docs.google.com/spreadsheets/d/e/2PACX-1vQe6sBfW-7S3xGPlYVaOB8v39yfZHx0FqCOeGEChuWlkObw-F5EsuVag_olya-psWYyKOuCl9y8ZGcf/pub?gid=1136921178&single=true&output=csv",
    // Uikit
    "https://docs.google.com/spreadsheets/d/e/2PACX-1vQe6sBfW-7S3xGPlYVaOB8v39yfZHx0FqCOeGEChuWlkObw-F5EsuVag_olya-psWYyKOuCl9y8ZGcf/pub?gid=268501364&single=true&output=csv",
+    // Settings


Est-ce qu'on a une update par rapport à TI/la sécurité en utilisant google sheet pour la localisation ?

VincentErb · 2025-02-12T15:28:08Z

README.md

@@ -12,13 +12,14 @@ The goal is to standardize how we build web projects in the company.



On veut peut-être donner une version spécifique de Yarn à viser. On peut peut-être aussi linker vers la doc qui est dans /frontend/ directement, plutôt que donner une première étape ici mais le reste dans l'autre README

VincentErb · 2025-02-12T15:29:40Z

frontend/README.md

-
-To see the logs directly in your console, you can remove the -d flag. Otherwise, you can view output in Docker Desktop.
-
-## Update locale files


Je pense qu'on pourrait conserver cette section, ainsi que la section "File structure" en dessous

Luis Doebbel added 7 commits January 29, 2025 15:06

complete rewrite using react router and react query

1ab39c7

docs for data fetching and routing

ab13fb1

doc updates

37e9eae

updates docs, adds vscode workspace configuration

712dbc6

Vite config adjusted to avoid errors on very first load

8fd153e

login page rewrite with clientAction and fetcher.

245d439

Updated translations.

more code refactor, localized routes, update to docs.

cb249d2

luisdlopez requested review from Seeila and VincentErb February 6, 2025 21:26

Seeila reviewed Feb 10, 2025

View reviewed changes

VincentErb reviewed Feb 12, 2025

View reviewed changes

	The alias here is different than the one define in the tsconfig file. This alias comes from the `imports` property in the package.json file.
	The alias here is different than the one defined in the tsconfig file. This alias comes from the `imports` property in the package.json file.


		This ensures that the dashboard route is only accessible to authenticated users.

		At the moment, react rouvet v7 does not support middlewares (it is planned though). You have a couple of options as to where to use the `clientLoaderAuthGuard`:

	At the moment, react rouvet v7 does not support middlewares (it is planned though). You have a couple of options as to where to use the `clientLoaderAuthGuard`:
	At the moment, react router v7 does not support middlewares (it is planned though). You have a couple of options as to where to use the `clientLoaderAuthGuard`:

		@@ -12,13 +12,14 @@ The goal is to standardize how we build web projects in the company.


		To see the logs directly in your console, you can remove the -d flag. Otherwise, you can view output in Docker Desktop.

		## Update locale files

Conversation

luisdlopez commented Feb 6, 2025

Proposed Changes

What is the new behaviour?

Checklist

Other information

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants