Skip to content

[PA] Refonte de la vue "Tous les plans d'action" #3830

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

Merged

Conversation

GaelS
Copy link
Contributor

@GaelS GaelS commented Jun 26, 2025

No description provided.

@GaelS GaelS changed the base branch from main to ui-refactoring-detailed-plan-d-action-view June 26, 2025 15:38
Copy link
Contributor

Choose a reason for hiding this comment

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

Juste en survolant les fichiers, je vois que tu as re-créé un dossier /src/plans/plans.
Sur la PR TDB j'en ai créé un /src/plans-action/plan avec une list de plan et la carte. T'avais pu rebase ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah oui mince j'avais pas catché ça dans ta PR @cparthur , mais j'avais justement vu avec @GaelS pour nommer le domaine et son sous-domaine plans/plans pour être homogène avec le naming "officiel" des domaines, et cohérent avec le backend qui utilise ça.

@GaelS GaelS force-pushed the tous-les-plan-d-action-view branch from 613bd9c to 5a41acc Compare June 30, 2025 08:32
Copy link
Contributor

@farnoux farnoux left a comment

Choose a reason for hiding this comment

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

Globalement super en terme de structure, très propre 👍

J'aimerais juste qu'on s'accorde sur les nommages de dossiers features et composants à l'intérieur pour poser une base pérenne pour la suite.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah oui mince j'avais pas catché ça dans ta PR @cparthur , mais j'avais justement vu avec @GaelS pour nommer le domaine et son sous-domaine plans/plans pour être homogène avec le naming "officiel" des domaines, et cohérent avec le backend qui utilise ça.

panierId: string | undefined;
};

export const AllPlansView = ({ plans, collectiviteId, panierId }: Props) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Sur le naming, je trouverais plus simple à lire (et cohérent de bout en bout avec les naming features côté backend) d'avoir toujours le verbe d'action en premier, i.e. :

  • list-all-plans
  • show-detailed-plan

Copy link
Contributor

Choose a reason for hiding this comment

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

Je m'interroge sur l'utilité d'un sous-dossier components pour chaque dossier feature ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Idem, ça a du sens dans l'app router pour faciliter la lisibilité des routes, pour le reste c'est surement trop poussé, si on commence à avoir trop de composants au niveau du parent (disons 4 et plus), alors c'est surement que l'on peut créer un sous-dossier pour combiner des composants.

Copy link
Contributor

Choose a reason for hiding this comment

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

naming plutôt en nom-du-compo.type.tsx, i.e. plan-card.list.tsx

Copy link
Contributor

Choose a reason for hiding this comment

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

naming, plutôt create-plan.button.tsx

@GaelS GaelS force-pushed the tous-les-plan-d-action-view branch from 5a41acc to 8c175de Compare June 30, 2025 10:23
Copy link
Contributor

Choose a reason for hiding this comment

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

À discuter en équipe, je suis plutôt partant de toujours nommer les fichiers comme leurs dossiers.
Ici avoir detailed-plan-action-view/detailed-plan-action.view.tsx

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah yes, c'est cool ça!

@GaelS GaelS force-pushed the tous-les-plan-d-action-view branch 2 times, most recently from efdee0c to 7f8f2bb Compare June 30, 2025 13:33
@GaelS GaelS force-pushed the tous-les-plan-d-action-view branch from 7f8f2bb to 3d63a3b Compare June 30, 2025 13:56
@GaelS GaelS force-pushed the tous-les-plan-d-action-view branch from 48cfb4f to 6ce20a4 Compare June 30, 2025 15:25
@GaelS GaelS merged commit f4a2e33 into ui-refactoring-detailed-plan-d-action-view Jun 30, 2025
12 checks passed
@GaelS GaelS deleted the tous-les-plan-d-action-view branch June 30, 2025 15:33
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.

3 participants