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: nuxt #1304

Draft
wants to merge 129 commits into
base: main
Choose a base branch
from
Draft

Feat: nuxt #1304

wants to merge 129 commits into from

Conversation

yvalentin
Copy link
Collaborator

No description provided.

# Conflicts:
#	package-lock.json
#	package.json
…rked extension. fix version of tsoa and dependency lib
# Conflicts:
#	package-lock.json
# Conflicts:
#	package-lock.json
# Conflicts:
#	package-lock.json
# Conflicts:
#	apps/nuxt/src/components/questionnaire/track/TrackContent.vue
#	apps/nuxt/src/tools/storage/companyDataStorage.ts
#	apps/web-e2e/src/formResults.spec.ts
#	apps/web-e2e/src/formResultsData.ts
#	apps/web/src/components/project/details/ProjectPrograms.vue
# Conflicts:
#	apps/nuxt-e2e/src/program/programResults.spec.ts
#	apps/nuxt/src/components/identification/details/TeeProfileDetails.vue
#	apps/nuxt/src/stores/project.ts
#	libs/data/src/program/yamlGenerator/publicodesGenerator.ts
Copy link
Collaborator

@ttdm ttdm left a comment

Choose a reason for hiding this comment

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

Gros gros gros taf !

  • L'archi de "page" est très très claire !
  • TODO supprimer le if:false de playwright.yml ds github/workflow
    Et valider les tests playwrights avant mise en prod
  • dans les middleware, renommer le fichier hook en common. middleware = hook, donc ne pas mettre 2 noms différents.
  • j'y pense ici : tu as testé sentry en créant volontairement des erreurs sur le front comme sur le back ?
  • note pour moi même : a tester les scripts du package back.

libs/data/src/project/types/shared.ts Show resolved Hide resolved
Copy link
Collaborator

@ttdm ttdm Dec 20, 2024

Choose a reason for hiding this comment

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

Suggestion: rennommer ce fichier et la classe en MiddlewareBase // MiddlewareUtils ..
En tout cas supprimer le terme hook qui, sur les questions de route, se nomme middleware sur nuxt.
Peut être docummenter le naming quelque part ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Question : c'est quoi ? :)
(Aussi une note pour moi même d'aller creuser après la review)


definePageMeta({
name: RouteName.CatalogPrograms,
middleware: [MiddlewareName.resetUsedTrackStore, MiddlewareName.resetQueries]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Ici il doit manquer : Hook.resetProgramFilters ?


const urls: SitemapUrlInput[] = []
for (const programId of programsIds) {
const isInclude = activeProgramsIds.includes(programId)
Copy link
Collaborator

Choose a reason for hiding this comment

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

MIni comment : isActive serait plus clair. ou isValid

Copy link
Collaborator

Choose a reason for hiding this comment

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

SI je comprend bien tu as séparé ce fichier dans src/stores/navig et src/tools/navig. Tu peux nous en dire plus sur l'archi visée ?

import type { ProgramData, QuestionnaireData } from '@/types'
import { Result } from 'true-myth'
import RequestApi from '@/tools/api/requestApi'
import { ResultApi } from '@/tools/api/resultApi'
Copy link
Collaborator

@ttdm ttdm Dec 20, 2024

Choose a reason for hiding this comment

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

Je le mets la parce que c'est trop galère de retrouver le fichier en question sur github mais praise pour avoir sorti Result dans un fichier. et du coup être quasi transparent vis à vis de la lib dans le front.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm, le naming en statApi au singulier importé comme StatsApi au pluriel m'étonne.
Toutes les autres API ont le même nom de class et de fichier.
1/ je veux bien une expliatoin de pourquoi ça fonctionne !
2/ ça me semble pas mal de rename StatApi en StatsApi non ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Discuté, ça fonctionne car export "default", pk pas le renaming quand même :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

A déplacer ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

D'habitude on passe par le NAF1LEtters pour ça.
Après ça me va, c'est juste pour signaler que ça fait peut être doublon du coup ;)

@yvalentin yvalentin changed the base branch from release/nuxt to main December 20, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants