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

Mise à jour du paquet mesaidesvelo avec le nouveau paquet @betagouv/aides-velo #4705

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

Conversation

Shamzic
Copy link
Contributor

@Shamzic Shamzic commented Nov 5, 2024

Reprise de la PR de EmileRolley #4695

@github-actions github-actions bot added this to the BC actuel milestone Nov 5, 2024
@Shamzic Shamzic changed the title Mise à jour du paquet mesaidesvelo avec le paquet betagouv Mise à jour du paquet mesaidesvelo avec le nouveau paquet @betagouv/aides-velo Nov 5, 2024
@Shamzic Shamzic changed the title Mise à jour du paquet mesaidesvelo avec le nouveau paquet @betagouv/aides-velo Mise à jour du paquet mesaidesvelo avec le nouveau paquet @betagouv/aides-velo [WIP]  Nov 5, 2024
@Shamzic Shamzic changed the title Mise à jour du paquet mesaidesvelo avec le nouveau paquet @betagouv/aides-velo [WIP]  Mise à jour du paquet mesaidesvelo avec le nouveau paquet @betagouv/aides-velo [WIP]  Nov 5, 2024
@Shamzic Shamzic force-pushed the refactor-aides-velo branch 6 times, most recently from d67ba47 to 770a0e0 Compare November 7, 2024 15:27
@Shamzic
Copy link
Contributor Author

Shamzic commented Nov 8, 2024

Je me suis permis quelques refacto @EmileRolley @jenovateurs preneur de vos retours (:

Copy link
Contributor

@jenovateurs jenovateurs left a comment

Choose a reason for hiding this comment

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

Premier retour sur certains aspects. Merci pour cette intégration !

data/benefits/aides-velo-generator.ts Show resolved Hide resolved
tests/integration/benefits-description.spec.ts Outdated Show resolved Hide resolved
@Shamzic Shamzic requested a review from jenovateurs November 22, 2024 09:35
Copy link
Contributor

@jenovateurs jenovateurs left a comment

Choose a reason for hiding this comment

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

En attente du merge de la PR betagouv/publicodes-aides-velo#18
pour valider cette PR.
@EmileRolley aurais-tu un peu de temps pour y jeter un œil ?
Merci d'avance.

@Shamzic Shamzic changed the title Mise à jour du paquet mesaidesvelo avec le nouveau paquet @betagouv/aides-velo [WIP]  Mise à jour du paquet mesaidesvelo avec le nouveau paquet @betagouv/aides-velo Nov 27, 2024
@Shamzic Shamzic force-pushed the refactor-aides-velo branch 2 times, most recently from 6f13853 to 3046480 Compare November 27, 2024 16:36
Copy link
Contributor

@jenovateurs jenovateurs left a comment

Choose a reason for hiding this comment

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

Présence de doublon dans les noms d'aide, on dirait un bug. Des idées pour son amélioration ?
Screenshot 2024-11-29 at 11 31 13
Screenshot 2024-11-29 at 11 31 54

Screenshot 2024-11-29 at 11 34 45

Si je suis les règles établit dans betagouv/aides-velo, les dispositifs PMR :
Vélo adapté pour PMR
Nécessite en tout cas pour l'Hérault (j'ai pas vérifié les autres) d'être en situation de handicap. Ma suggestion est d'ajouter la checkbox dans ce cas là. Qu'en penses-tu ? @Shamzic

@Shamzic
Copy link
Contributor Author

Shamzic commented Nov 29, 2024

Présence de doublon dans les noms d'aide, on dirait un bug. Des idées pour son amélioration ?

Ce ne sont pas des doublons mais des "variations" d'une aide avec le même nom qu'il faudrait mettre à jour, ou bien fusionner ces aides sur @betagouv/publicode-aides-velo

Nécessite en tout cas pour l'Hérault (j'ai pas vérifié les autres) d'être en situation de handicap. Ma suggestion est d'ajouter la checkbox dans ce cas là. Qu'en penses-tu ? @Shamzic

Je n'ai pas compris où est-ce que tu veux ajouter une checkbox ?

@@ -109,7 +109,7 @@ describe("benefit descriptions", function () {
.replace(/\s\s+/g, " ")
.trim()
expect(innerText.length).toBeGreaterThanOrEqual(10)
expect(innerText.length).toBeLessThanOrEqual(600)
expect(innerText.length).toBeLessThanOrEqual(450)
Copy link
Contributor

Choose a reason for hiding this comment

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

Il faudrait mettre à jour cette information sur l'outil de contribution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

J'ai oublié de préciser; pourra laisser à 420 (comme avant) après le merge de cette modif https://github.com/betagouv/publicodes-aides-velo/pull/20/files

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok trop bien alors si on peux remettre 420.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, d'ailleurs nous avons maintenant accès aux review/merge sur le répo publicodes-aides, si ça te dit (:

@jenovateurs
Copy link
Contributor

Je n'ai pas compris où est-ce que tu veux ajouter une checkbox ?

Screenshot 2024-12-02 at 10 32 34

Pour moi, si on n'est pas en situation de handicap, on supprime la checkbox.

@jenovateurs
Copy link
Contributor

À tester si le problème inverse n'est pas présent. Je suis en situation de handicap et je coche juste « adapté pour PMR » ; est-ce que les aides d'achat génériques sont affichées quand même, car j'ai l'impression que l'aide PMR est un bonus supplémentaire à l'aide de base.

@Shamzic
Copy link
Contributor Author

Shamzic commented Dec 3, 2024

Pour moi, si on n'est pas en situation de handicap, on supprime la checkbox.

Ok. J'ai restreint à la condition d'affichage dans mon dernier commit pour que le demandeur soit en situation de handicap.

Cependant, cela pose soucis pour le cas où le demandeur n'est pas en situation de handicap mais qu'il a un enfant à sa charge qui est en situation de handicap. Ici, la case ne sera pas affichée mais est-ce qu'il serait quand même bénéficiaire d'une aide vélo pour PMR dû au handicap de son enfant ?

@Shamzic
Copy link
Contributor Author

Shamzic commented Dec 3, 2024

À tester si le problème inverse n'est pas présent. Je suis en situation de handicap et je coche juste « adapté pour PMR » ; est-ce que les aides d'achat génériques sont affichées quand même, car j'ai l'impression que l'aide PMR est un bonus supplémentaire à l'aide de base.

Il y a deux aides "génériques" (si je comprends bien), c'est le bonus vélo et celle de la région Île-de-France. Elles sont bien affichées lorsque l'utilisateur coche uniquement "vélo adapté", la description est dynamique sur le type de vélo choisi.

Le problème est le suivant : sur mes mesaidesvelo.fr l'affichage est conditionné par un unique type de vélo (ex: la page dédiée aux aides du vélo mécanique et la page spécifique pour les aides au vélo électrique). Sur Aides Jeunes, on a le soucis d'avoir les aides de tous les types de vélo affichées ensembles et cela peut porter à confusion sur l'aide bonus vélo car elle n'est affichée qu'une fois avec un seul type de vélo, malgré le fait que l'utilisateur en ait coché plusieurs (le type générique affiché dans la description est le dernier de la liste des types de vélo transmise à mesaidesvelo).

@Shamzic Shamzic force-pushed the refactor-aides-velo branch from 08d535e to d2ece8c Compare December 3, 2024 16:13
@Shamzic
Copy link
Contributor Author

Shamzic commented Dec 3, 2024

@jenovateurs Cette suggestion permet de préciser les types de vélo proposés par l'aide (via la description mesaidesvelo, pas d'autre solution) pour éviter les doublons (cf ton screen plus haut). C'est plus précis mais le petit inconvénient à cela c'est que le titre de l'aide est un peu long pour certains cas. Qu'en pense-tu ?

image

@jenovateurs
Copy link
Contributor

Merci pour la prise en compte de ma suggestion.
Pour gérer le problème de longueur, peux-t-on supprimer le nom de la ville, car on a l'info avec le sous-titre ou l'élément parent.
Screenshot 2024-12-06 at 17 28 04
Qu'en penses-tu ?

@Shamzic
Copy link
Contributor Author

Shamzic commented Dec 9, 2024

Edit : Erreur de manip

@Shamzic Shamzic closed this Dec 9, 2024
@Shamzic Shamzic reopened this Dec 9, 2024
@Shamzic
Copy link
Contributor Author

Shamzic commented Dec 9, 2024

Merci pour la prise en compte de ma suggestion. Pour gérer le problème de longueur, peux-t-on supprimer le nom de la ville, car on a l'info avec le sous-titre ou l'élément parent. Qu'en penses-tu ?

Je suis ok, c'est modifié.

@Shamzic Shamzic requested a review from jenovateurs December 9, 2024 16:18
@jenovateurs
Copy link
Contributor

Il y a encore des descriptions trop longues sinon c'est top @Shamzic .
Concernant le raccourcissement des descriptions, c'est en cours sur le repo dédié ?

@Shamzic
Copy link
Contributor Author

Shamzic commented Dec 11, 2024

Il y a encore des descriptions trop longues sinon c'est top @Shamzic . Concernant le raccourcissement des descriptions, c'est en cours sur le repo dédié ?

Zut, il y a encore la description de aidesvelo_aides_béthune_bruay qui est trop longue..

@Shamzic
Copy link
Contributor Author

Shamzic commented Dec 12, 2024

En attente du merge de betagouv/publicodes-aides-velo#27 qui désactive l'aide aidesvelo_aides_béthune_bruay car elle n'est plus valide

@EmileRolley
Copy link

En attente du merge de betagouv/publicodes-aides-velo#27 qui désactive l'aide aidesvelo_aides_béthune_bruay car elle n'est plus valide

Je peux merge demain si vous voulez !

@jenovateurs
Copy link
Contributor

jenovateurs commented Dec 18, 2024

Merci pour le merge Emile et pour l'update Simon.
Il y a de nouveau un autre élément trop long. Je te propose d'itérer en local avec le package chargé et qu'on vérifie la CI jusqu'au bout. Qu'en penses-tu @Shamzic ?

@jenovateurs
Copy link
Contributor

Pour info : j'ai ajouté notre test de taille de caractère du champ description : betagouv/publicodes-aides-velo#31
Donc en attente de relecture/merge pour retester notre CI.

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