-
Notifications
You must be signed in to change notification settings - Fork 24
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
Les pages enfantes rendent à présent leur élément correspondant du menu actif [GEN-1956] #5017
base: master
Are you sure you want to change the base?
Conversation
🥁 La recette jetable est prête ! 👉 Je veux tester cette PR ! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Je ne suis pas fan de se baser sur des regexps, car on perd la possibilité de lister les vues en question (grep, dans les tests, trouver les doublons). On se retrouve à créer des groupes de vues via des regexps, je pense qu’on devrait grouper autrement (par exemple, en annotant les vues pour les rattacher à leur menu). Ça aurait l’avantage de déclarer la vue dans son menu.
J’avais mis boost parce que je pensais ajouter les éléments dans la liste. Si ce n’est pas possible / ça devient trop pénible, je pense qu’on devrait mettre en place un système plus clair de lien page <-> menu
.
Pas trop fan non plus des regex mais je verrais bien un système vérifiant que toutes les vues de |
Je vais proposer une nouvelle version. |
5c2318c
to
f5cadf0
Compare
15434b3
to
b5c6007
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C'est très très verbeux, mais avec la présence de test_all_urls_registered
c'est simple et sûr.
Je ne peux pas approve, mais le coeur y est.
Tu pourras rebase et virer mon commit 🙏
"companies_views:edit_job_description_preview", | ||
"companies_views:job_description_list", | ||
"companies_views:select_financial_annex", | ||
"companies_views:update_job_description", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cf. mon commentaire en description de PR : est-ce que l'on souhaite vraiment lier des vues concernant la création/modification d'entreprise ou les AF à l'item "Métiers et recrutements" ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pour les AF, on a une catégorie à part, je viens de déplacer "companies_views:select_financial_annex"
dans cette section.
Pour la création d’entreprise, on n’a pas mieux et ça fait partie du recrutement 🤷
b5c6007
to
edaec09
Compare
excluded_namespace_urls.add(namespaced_url) | ||
expected_urlnames_in_nav = excluded_namespace_urls - not_html_urlnames - excluded_urlnames | ||
nav_urlnames = { | ||
active_view_name for entry in NAV_ENTRIES.values() for active_view_name in entry.active_view_names |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Idéalement il faudrait le faire pour chaque type d'utilisateur mais ça rajouterait encore plus de complexité donc ça me semble déjà bien :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
C’est complexe, car chaque utilisateur n’a pas accès à toutes les vues...
Il faudrait pouvoir décrire qui a accès à quelle vue, dans quelles les conditions. J’ai pensé forcer l’inclusion d’éléments spécifiques au menu dans le contexte de chaque vue, mais ça rend la vérification de la nav plus pénible car il faudrait valider les conditions d’accès à la vue.
Comme un bug cause juste de ne pas rendre actif un élément de la nav, on va rester au niveau de complexité actuel.
Pour info, avec #5051 qui est dans la merge queue, il y a 2 URLs en plus : |
Merci 😊 ! Aucun souci, ça va casser les tests et on va pouvoir les rajouter aux bons endroits. |
Cela dit, ça amène une question intéressante : est-ce qu’on déplace ces pages sous l’espace « mes candidats » ou on les laisse sous « Candidatures » ? |
edaec09
to
52137d4
Compare
🤔 Pourquoi ?
Toujours activer la page d’origine lorsque je descends en profondeur dans le site
Il y a toujours un item du menu actif (en gras bleu .nav-link.active) sauf pour mon espace et besoin d’aide
🍰 Comment ?
Ne plus matcher une liste de vues arbitraire en permettant un match partiel avec
re
🏝️ Comment tester
En recette jetable, tester les différents parcours pour chaque type de profil (candidat, employeur, prescripteur, institution).
Commentaires
edit-company
) qui pourraient activer leNavGroup
structure, mais comme ça ne correspond à aucun item de ce groupe, pour l'instant le menu reste inactifactive
sur les bons items💻 Captures d'écran