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(News): intégration "J'ai pris connaissance..." + changes #281

Merged
merged 33 commits into from
Nov 26, 2024

Conversation

Kgeek33
Copy link
Contributor

@Kgeek33 Kgeek33 commented Oct 9, 2024

Cette PR n'est pas en rapport avec #271

🚀 Nouvelle Pull Request

Proposez vos modifications pour améliorer Papillon

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

  • Ajout d"une CheckBox "J'ai lu et pris connaissance" lorsqu'une actualité demande un accusé de réception (en gros)
  • Conversion variable message -> useState
  • Permet un affichage dynamique du texte "Marquer comme (non) lu"
  • Changement dynamique de ce useState lorsqu'on clique sur ce bouton
  • Indentation du code effectué par Prettier
  • Correction filtrage de Papillon Magic qui supprimait des actualités

Informations supplémentaires

  1. Pronote interdit de décocher une actualité déjà accusé de réception. Du coup, il est impossible de décocher la checkBox
  2. Screenshot_2024-10-09-23-44-21-683_host.exp.exponent.jpg
Screenrecorder-2024-10-09-23-44-05-191.mp4

Closed #275
Intégration de certains éléments de #325

@yannouuuu yannouuuu added ✨ enhancement New feature or request 🚸 user experience UX related issues labels Oct 10, 2024
@Rexxt
Copy link
Contributor

Rexxt commented Oct 11, 2024

2 minutes je regarde tout ça

Copy link
Contributor

@Rexxt Rexxt left a comment

Choose a reason for hiding this comment

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

LGTM, c'est propre

src/views/account/News/Document.tsx Outdated Show resolved Hide resolved
src/views/account/News/Document.tsx Outdated Show resolved Hide resolved
@Kgeek33 Kgeek33 requested a review from Rexxt October 11, 2024 17:09
Copy link
Contributor

@Rexxt Rexxt left a comment

Choose a reason for hiding this comment

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

ça m'a l'air bon, LGTM

src/views/account/News/Document.tsx Outdated Show resolved Hide resolved
@tryon-dev
Copy link
Contributor

Mais pourquoi faire c'était déjà le cas nan ?

@Kgeek33
Copy link
Contributor Author

Kgeek33 commented Oct 13, 2024

@tryon-dev on pouvait juste marquer une actualité comme lue, pas la cocher dans certains cas

@Kgeek33
Copy link
Contributor Author

Kgeek33 commented Oct 15, 2024

Warning

Sur le fichier src/router/screens/views/index.ts, il faut préciser le type d'animation !. Après, on aura des issues comme #140

d'où le dernier commit "correction d'un affichage aléatoire"

@Kgeek33 Kgeek33 mentioned this pull request Oct 18, 2024
9 tasks
@Kgeek33
Copy link
Contributor Author

Kgeek33 commented Oct 19, 2024

@tryon-dev

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.

Ça n'a pas l'air de fonctionner chez moi...

@Gabriel29306
Copy link
Contributor

@ecnivtwelve tu pourrais re review la PR ?

@Gabriel29306
Copy link
Contributor

Toi aussi @Louis-htmlcss tu pourrais revoir ta review ?

@Louis-htmlcss
Copy link
Contributor

Toi aussi @Louis-htmlcss tu pourrais revoir ta review ?

C'est good

@Kgeek33 Kgeek33 requested a review from ecnivtwelve November 15, 2024 08:51
@ecnivtwelve
Copy link
Contributor

Il faut que je fasse une review supplémentaire, cependant le probléme étant que ton autre PR (#325) est si grosse qu'elle risque de break celle-ci, notamment vu les changements aux actualités.

@Kgeek33
Copy link
Contributor Author

Kgeek33 commented Nov 16, 2024

Je vais intégrer les changements de #325 directement dedans, comme ça, pas de risque de break

@Kgeek33 Kgeek33 requested a review from tryon-dev as a code owner November 16, 2024 21:41
@Kgeek33
Copy link
Contributor Author

Kgeek33 commented Nov 16, 2024

@ecnivtwelve @Vexcited
j'ai intégré des éléments de #325

@Rexxt
Copy link
Contributor

Rexxt commented Nov 23, 2024

TODO : review

@Louis-htmlcss
Copy link
Contributor

TODO : review

💀

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.

LGTM

@ecnivtwelve ecnivtwelve merged commit 0e7024a into PapillonApp:main Nov 26, 2024
1 check failed
@Kgeek33 Kgeek33 deleted the feat/accuseNews branch November 26, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement New feature or request 🚸 user experience UX related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Actualités
7 participants