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

Affichage du "numéro de semaine" dans l'EDT #545

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

Skythrew
Copy link
Contributor

@Skythrew Skythrew commented Dec 26, 2024

🚀 Nouvelle Pull Request

Informations importantes

Merci de vous référer à la documentation sur la contribution si vous avez des questions à propos des pull requests (https://gitbook.getpapillon.xyz/organisation/outils-internes/github)

Checklist d'avant pull request

Veuillez cocher toutes les cases applicables en remplaçant [ ] par [x].

  • Vous avez testé de build le projet avec vos modifications et ce build a réussi
  • Vous respectez les conventions de codage et de nommage du projet
  • Vous utilisez la tabulation pour l'indentation afin de maintenir un code lisible
  • Cette pull request n'est pas un duplicata d'une autre
  • Cette pull request est prête à être revue (review) et fusionnée (merge)
  • Il n'y a pas de TODO (aka des annotations pour du code manquant) dans vos modifications
  • Il n'y a pas d'erreurs de langue dans votre code (grammaire, vocabulaire, conjugaison, orthographe)
  • Les détails des changements ont été décrits ci-dessous
  • Cette pull-request n'est pas une "breaking-change" (des modifications qui vont entraîner la modification du fonctionnement de certaines fonctionnalités déjà existantes)

Changelogs proposés

Affichage de la "fréquence" de la semaine en cours sur la page d'emploi du temps. Typiquement, "Semaine Q1", "Semaine Q2".

@Skythrew Skythrew requested a review from Vexcited as a code owner December 26, 2024 18:00
@Skythrew Skythrew changed the title Feat/week frequency Affichage du "numéro de semaine" dans l'EDT Dec 26, 2024
@Kgeek33
Copy link
Contributor

Kgeek33 commented Dec 26, 2024

Peux-tu mettre une capture d'écran stp ?

@Kgeek33
Copy link
Contributor

Kgeek33 commented Dec 26, 2024

Et ça résout pas l'issue #469 ?

ecnivtwelve
ecnivtwelve previously approved these changes Dec 26, 2024
Copy link
Contributor

@ecnivtwelve ecnivtwelve left a comment

Choose a reason for hiding this comment

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

j'ai fait des changements d'UI + rendu l'option optionnelle

Copy link
Contributor

@Kgeek33 Kgeek33 left a comment

Choose a reason for hiding this comment

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

Change juste ça, sinon LGTM

src/views/account/Lessons/Lessons.tsx Outdated Show resolved Hide resolved
@Clmnnt
Copy link
Contributor

Clmnnt commented Dec 26, 2024

possible de faire un screen de ce que ça fait ?

Copy link
Contributor

@Kgeek33 Kgeek33 left a comment

Choose a reason for hiding this comment

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

Parfait LGTM 👍

@Kgeek33
Copy link
Contributor

Kgeek33 commented Dec 26, 2024

possible de faire un screen de ce que ça fait ?

Déso, mon pc est éteint 😅

@Kgeek33
Copy link
Contributor

Kgeek33 commented Dec 26, 2024

Mais ça ajoute dans un joli encadré la semaine
Genre si t'es en semaine q1, ça affichera dans un encadré à côté de la date Q1

@Skythrew
Copy link
Contributor Author

possible de faire un screen de ce que ça fait ?

ça donne ça dans le header de la page de l'emploi du temps:

Capture d’écran du 2024-12-27 10-53-20

@Skythrew
Copy link
Contributor Author

Et ça résout pas l'issue #469 ?

A priori non, ça a l'air d'être une donnée à part, vu que sur le screen donné dans l'issue on voit bien "Week Q2"

@ecnivtwelve
Copy link
Contributor

pas forcément pour l'activer par défaut, cependant on peut rendre l'option persistante dans l'account?

@Skythrew
Copy link
Contributor Author

C'est vrai, je peux m'occuper de ça si vous voulez

@Kgeek33
Copy link
Contributor

Kgeek33 commented Dec 27, 2024

ok pour rendre l'option persistante dans l'account
si l'option doit être désactivé par défaut, alors faudrait trouver un moyen pour mettre cette option à l'avant pask elle ne peut ne pas être facile à trouver (mais ça, peut-être le faire dans une autre pr)

@ecnivtwelve
Copy link
Contributor

ok pour rendre l'option persistante dans l'account si l'option doit être désactivé par défaut, alors faudrait trouver un moyen pour mettre cette option à l'avant pask elle ne peut ne pas être facile à trouver (mais ça, peut-être le faire dans une autre pr)

après c'est une feature qu'on peut considérer comme "avancée", c'est pourquoi le but est de la laisser aux utilisateurs qui en veulent plus

@NathanBnm
Copy link

On est d'accord que le nom de la semaine est dynamique et récupéré sur Pronote ?

Vous parlez de Q1 et Q2 mais sur le compte que j'ai c'est A et B.

@Skythrew
Copy link
Contributor Author

Skythrew commented Jan 9, 2025

Yes évidemment

@godetremy
Copy link
Contributor

Note

Cette review est le résultat de la concertation de toute l'équipe Papillon.

Nous sommes d'accord pour valider cette PR, mais tu devrais faire un paramètre pour rendre cette option optionnelle (désactivée par défaut).

Nous revérifiions cette PR la semaine prochaine, si cela est corrigé, ta PR sera merge. ✌️

@Skythrew
Copy link
Contributor Author

C'est bon pour moi. L'option a bien été rendue persistante et est désormais désactivée par défaut.

Copy link
Contributor

@Kgeek33 Kgeek33 left a comment

Choose a reason for hiding this comment

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

Juste ça

src/views/account/Lessons/Lessons.tsx Show resolved Hide resolved
@Kgeek33
Copy link
Contributor

Kgeek33 commented Jan 12, 2025

Je review plus tard

Copy link
Contributor

@Kgeek33 Kgeek33 left a comment

Choose a reason for hiding this comment

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

okay t'as des erreurs eslint et une erreur sur la déclaration du typage (détail) mais fonctionne bien

src/services/pronote/timetable.ts Outdated Show resolved Hide resolved
src/services/pronote/timetable.ts Outdated Show resolved Hide resolved
src/services/pronote/timetable.ts Outdated Show resolved Hide resolved
src/services/timetable.ts Outdated Show resolved Hide resolved
src/views/account/Lessons/Lessons.tsx Outdated Show resolved Hide resolved
src/services/shared/Timetable.ts Outdated Show resolved Hide resolved
@Skythrew
Copy link
Contributor Author

Les erreurs restantes ESLint ne sont pas du ressort de cette PR et sont d'ailleurs résolues par mon autre PR #637 .

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.

[Feature]: Afficher le numéro de la semaine sur l'EDT
6 participants