Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat/init paradata #154

Merged
merged 1 commit into from
Nov 19, 2024
Merged

Feat/init paradata #154

merged 1 commit into from
Nov 19, 2024

Conversation

chloe-renaud
Copy link
Contributor

No description provided.

@chloe-renaud chloe-renaud force-pushed the feat/init-paradata branch 2 times, most recently from 69762b7 to 8bc83ad Compare October 31, 2024 16:04
@chloe-renaud chloe-renaud marked this pull request as ready for review October 31, 2024 16:04
.env Outdated Show resolved Hide resolved
src/shared/components/Layout/Header.test.tsx Show resolved Hide resolved
src/shared/components/Layout/Header.test.tsx Show resolved Hide resolved
src/shared/components/Orchestrator/Orchestrator.test.tsx Outdated Show resolved Hide resolved
src/types/telemetry.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@laurentC35 laurentC35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, but I've make some comments.

package.json Outdated Show resolved Hide resolved
src/App.tsx Outdated Show resolved Hide resolved
src/App.tsx Outdated Show resolved Hide resolved
.env Outdated Show resolved Hide resolved
src/main.tsx Outdated Show resolved Hide resolved
src/model/SurveyUnitData.ts Outdated Show resolved Hide resolved
src/router/router.tsx Outdated Show resolved Hide resolved
src/shared/components/Layout/Header.tsx Show resolved Hide resolved
}
}
triggerDataAndStateUpdate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ici on va trigger l'update sans condition (là où avant on vérifiait qu'il y avait de la donnée modifiée), ça me paraît pas problématique mais à avoir en tête

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oui garder la logique qu'on avait avant serait mieux ^^

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

finalement c'était le seul cas qui vérifiait, le logout trigger sans condition (alors que comme ici on n'a potentiellement rien à modifier car on reste sur la même page) => pas très cohérent
Le seul cas qui ne nécessite pas de vérification est le changement de page (on veut a minima update le currentPage), pour les autres est-ce nécessaire... ?

src/shared/components/Layout/Header.tsx Show resolved Hide resolved
@chloe-renaud chloe-renaud requested a review from nsenave November 15, 2024 13:22
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Petite question, il y a un réel gain à faire 2 useEffect au lieu d'un seul ? (j'ai pas la réponse)

src/shared/components/Layout/Header.tsx Show resolved Hide resolved
}
}
triggerDataAndStateUpdate()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oui garder la logique qu'on avait avant serait mieux ^^

@chloe-renaud chloe-renaud requested a review from QRuhier November 15, 2024 17:17
Copy link

@nsenave nsenave left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review en diagonale (la PR est un peu longue 🥵 )

Sur le truc de la constante à gauche dans les comparaisons, je l'ai pas relevé partout, me semble que c'est une bonne pratique à introduire dans les nouveaux devs (sous réserve que vous trouviez ça pertinent), mais pas matière à refacto toute l'appli en ce sens imo

src/contexts/TelemetryContext.tsx Show resolved Hide resolved
src/i18n/resources/en.tsx Show resolved Hide resolved
src/shared/components/Layout/Header.tsx Show resolved Hide resolved
website/src/pages/changelog.md Outdated Show resolved Hide resolved
Copy link
Contributor

@laurentC35 laurentC35 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 👍

@chloe-renaud chloe-renaud changed the base branch from main to dev November 19, 2024 08:54
@chloe-renaud chloe-renaud merged commit 5466608 into dev Nov 19, 2024
9 checks passed
@chloe-renaud chloe-renaud deleted the feat/init-paradata branch November 19, 2024 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants