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

[FEATURE] Ajouter la nouvelle barre de navigation dans Pix Admin (PIX-16550) #11404

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from

Conversation

er-lim
Copy link
Contributor

@er-lim er-lim commented Feb 12, 2025

🥞 Problème

La nouvelle barre de navigation n'était pas encore utilisée sur Pix Admin

🥓 Proposition

L'ajouter avec un feature toggle (FT_PIX_ADMIN_NEW_SIDEBAR_ENABLED) le temps que ça soit validé fonctionnellement.

🧃 Remarques

  • On en profite pour mettre un logo pour Pix Admin 😄

😋 Pour tester

En local

  • Ajouter dans votre .env côté API la variable FT_PIX_ADMIN_NEW_SIDEBAR_ENABLED=true
  • Vérifier que la nouvelle barre s'affiche.
  • Faire des tests de non régression sur ça.

En RA

  • Se connecter à la RA de Pix Admin
  • Vérifier qu'il n'y a pas de régressions sur les pages

@er-lim er-lim added the cross-team Toutes les équipes de dev label Feb 12, 2025
@er-lim er-lim self-assigned this Feb 12, 2025
@pix-bot-github
Copy link

Une fois les applications déployées, elles seront accessibles via les liens suivants :

Les variables d'environnement seront accessibles via les liens suivants :

@er-lim er-lim force-pushed the tech-add-pix-navigation-in-admin branch 4 times, most recently from a7018be to 808f67c Compare February 12, 2025 13:32
@er-lim er-lim changed the title [FEATURE] Ajouter la nouvelle barre de navigation dans Pix Admin (PIX-DUNNO) [FEATURE] Ajouter la nouvelle barre de navigation dans Pix Admin (PIX-16550) Feb 12, 2025
@AndreiaPena
Copy link
Member

Proposition mettre un aria-label avec le label en complet pour au moins le lire entièrement en lecteur d'écran Capture d’écran 2025-02-12 à 15 47 29

@AndreiaPena
Copy link
Member

AndreiaPena commented Feb 12, 2025

Proposition : sur la classe .page-body > remplacer padding par padding-top: var(--pix-spacing-3x);

Pour avoir ça :)
Capture d’écran 2025-02-12 à 15 52 57

Et sur .page-header un border-radius: 5px; :D

@AndreiaPena
Copy link
Member

AndreiaPena commented Feb 12, 2025

D'ailleurs Profil cible n'a pas de fond blanc dans sa section et je trouve le résultat pas si mal !

Capture d’écran 2025-02-12 à 15 57 06

On se rend compte que les blocs blancs prennent finalement beaucoup de place, et donc si on retire le padding, c'est déjà ça de gagné

Capture d’écran 2025-02-12 à 15 57 58

Argument pour faire passer ce menu en PROD : on peut gagner un chouilla d'espace :p

});

module('Target Profiles tab', function () {
module('when admin member have "SUPER_ADMIN", "SUPPORT" or "METIER" as role', function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

question: ici les 3 rôles sont questionnés en un seul test, et pour les parcours autonomes c'est splité. Une raison ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

c'est corrigé, j'ai splité tous les tests par cohérence !

admin/translations/en.json Outdated Show resolved Hide resolved
@service accessControl;

get userFullName() {
const adminMember = this.currentUser?.adminMember;
Copy link
Contributor

Choose a reason for hiding this comment

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

question: si on est pas connecté, on est pas censé voir ce composant non ?

Suggested change
const adminMember = this.currentUser?.adminMember;
const adminMember = this.currentUser.adminMember;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

En effet ! J'ai laissé car il faut sinon mettre à jour tous les tests d'acceptance pour mettre des firstname / lastname à chaque adminMember 😓

const screen = await render(<template><Sidebar /></template>);

// then
assert.dom(screen.getByRole('link', { name: 'Se déconnecter' })).exists();
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: stub le service session.invalidate et tester le click ?

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 enlevé l'action logout car le bouton ne fait qu'une redirection vers la page /logout

@er-lim er-lim force-pushed the tech-add-pix-navigation-in-admin branch 2 times, most recently from f6d2bf3 to 9a6537d Compare February 13, 2025 10:19
@Faraopix
Copy link
Contributor

Les entrées dans la nav sont très collées-serrées par rapport à ce que je vois sur storybook ou sur les autres app :
RA
pixui

J'ai fait un petit tour sinon, fonctionnellement je ne vois rien d'anormal 👍

@Faraopix
Copy link
Contributor

Et je ne sais pas si ça a un rapport mais je vois que "Type" ne s'affiche pas en entier :
Capture d’écran 2025-02-13 à 13 59 51

@QuentinChapelain-ui
Copy link

QuentinChapelain-ui commented Feb 13, 2025

Hello sur les différents retours :

  • Proposition mettre un aria-label avec le label en complet pour au moins le lire entièrement en lecteur d'écran : + 1 pour moi

  • Pour le titre en haut de page, on peut enlever le fond blanc il n'apporte rien et rends moins ISO l'interface avec les autres plateforme orga et certif. Pour consrver un spacing et faciliter la lecture on peut comme sur app ajouter une ligne / seperateur en dessous de 1px gris pix-neutral-100.

Capture d’écran 2025-02-13 à 15 49 44
  • Je rejoins le retour de Géraldine au sujet du spacing des items du menu, il y. effectivment un espace sur orga. C'est un ajustement local à admin ?
Capture d’écran 2025-02-13 à 15 45 10 Capture d’écran 2025-02-13 à 15 47 48

@er-lim er-lim force-pushed the tech-add-pix-navigation-in-admin branch from 7510ff7 to 8127766 Compare February 13, 2025 16:26
@er-lim er-lim force-pushed the tech-add-pix-navigation-in-admin branch from 8127766 to c1e0787 Compare February 13, 2025 16:35
@er-lim
Copy link
Contributor Author

er-lim commented Feb 13, 2025

Je fais un message groupé par rapport à tous vos retours (merci pour les feedbacks 🙏 ). J'en ai traité une partie :

Les entrées dans la nav sont très collées-serrées par rapport à ce que je vois sur storybook ou sur les autres app :

@Faraopix C'est corrigé !

Et je ne sais pas si ça a un rapport mais je vois que "Type" ne s'affiche pas en entier :

@Faraopix Non ça n'a pas de rapport, c'est le composant PixSelect qui n'a pas calculé la bonne taille par rapport aux options. Je ne l'ai pas traité 😛

Proposition mettre un aria-label avec le label en complet pour au moins le lire entièrement en lecteur

@AndreiaPena Fait !

Proposition : sur la classe .page-body > remplacer padding par padding-top: var(--pix-spacing-3x);
Et sur .page-header un border-radius: 5px; :D

@AndreiaPena C'est corrigé 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cross-team Toutes les équipes de dev 👀 Design Review Needed 👀 Func Review Needed Need PO validation for this functionally 👀 Tech Review Needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants