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

Ux/dat 409/new design authorization request card #373

Merged

Conversation

Isalafont
Copy link
Contributor

@Isalafont Isalafont commented Aug 21, 2024

Capture d’écran 2024-08-26 à 16 44 14


Capture d’écran 2024-08-26 à 16 44 48


Full Page

screencapture-localhost-3000-tableau-de-bord-organisation-2024-08-26-16_42_21 (1)


Quand l'utilisateur est dans la section Toutes celles de l'organisation, le nom du demandeur de la demande (autre que celle du current user) s'affiche.
Capture d’écran 2024-08-26 à 17 27 48

Copy link

linear bot commented Aug 21, 2024

@Isalafont Isalafont force-pushed the ux/DAT-409/new-design-authorization-request-card branch 2 times, most recently from 3f15988 to 9c8ab21 Compare August 22, 2024 12:11
Copy link
Contributor

@JeSuisUnCaillou JeSuisUnCaillou left a comment

Choose a reason for hiding this comment

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

Beau boulot déjà !

@Isalafont
Copy link
Contributor Author

Isalafont commented Aug 26, 2024

@evaspae Pourras tu me valider l'ordre de présentation des habilitation sur le Dasboard (Demandes à modifier, Demandes en cours d'instruction, demandes en brouillon, Mes habiliations)
J'ai trouvé cet ordre présent sur la page "Accueil" du Figma, mais trouvé un ordre de présentation différent dans une autre page du Figma 🫣

@Isalafont Isalafont force-pushed the ux/DAT-409/new-design-authorization-request-card branch from f03faea to 99ee506 Compare August 26, 2024 14:41
@Isalafont Isalafont force-pushed the ux/DAT-409/new-design-authorization-request-card branch from 99ee506 to 497339a Compare August 26, 2024 14:57
  - When current user goes to dashboard and can display all organization's authorization request
  it will display the full name of other applicants.
Copy link
Contributor

@jbfeldis jbfeldis left a comment

Choose a reason for hiding this comment

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

GG, c'est stylé !

J'ai laissé quelques commentaires plutôt axés sur le ruby dans erb, ça rejoint surtout les commentaires de Valentin sur l'utilisation plus poussée des decorators

Copy link
Contributor

@JeSuisUnCaillou JeSuisUnCaillou left a comment

Choose a reason for hiding this comment

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

@Isalafont Pour moi on est bons, je laisserait @jbfeldis valider après avoir échangé avec toi sur sa review.

@Isalafont Isalafont force-pushed the ux/DAT-409/new-design-authorization-request-card branch from c1295cd to 4a679f8 Compare August 27, 2024 12:29
@JeSuisUnCaillou
Copy link
Contributor

Moi j'ai déjà dit que ça m'allait.
Je vois qu'il reste des commentaires de @jbfeldis non resolus, vous en avez parlé à l'oral ?

@Isalafont
Copy link
Contributor Author

J'ai fait des commentaires mais ils sont restés en pending... ARGHHH

@Isalafont
Copy link
Contributor Author

@jbfeldis

on ne pourrait pas utiliser la forme de render qui fonctionne avec une collection ? ça éviterait de répéter les conditions vu qu'on est dans le cas ou rendre une collection vide ça fait exactement ce qu'on veut

J'ai tenté d'utiliser une collection mais j'ai l'impression que ça complexifie le code également au niveau du fichier app/views/dashboard/_authorization_requests.html.erb pour traiter la collection.
Les habilitations ne se classent plus dans leurs sections respectives.
Je pense que ça meriterait peut être de s'y plonger d'avantage dans un ticket de refacto ?

@Isalafont
Copy link
Contributor Author

@jbfeldis

on peut aussi pousser ça dans le decorator pour se débarrasser des conditions dans les vues (ça vaut pour la plupart des conditions qui suivent dans ce partial)

je n'ai pas regardé les decorators dans le projet donc ce n'est ptet pas aligné avec le style du projet mais on pourrait éventuellement en discuter

(bon moi j'avoue que j'aime pas les conditions et encore moins dans les vues 👴 😄 )

Jusqu'a présent dans le décorator, il y a surtout des methods helpers, je ne suis pas certaine que de déplacer une condition a une seule branche soit réellement un plus niveau lisibilité ?

Mais je suis d'accord qu'il y a peut être des questions à se poser quand à notre utilisation des decorators et ce que l'on doit y mettre ou pas ?

J'ai déplacé dans le decorator la condition précédente qui pour le coup améliore la lisibilité.

@JeSuisUnCaillou JeSuisUnCaillou merged commit 66cecb1 into develop Aug 28, 2024
16 checks passed
@JeSuisUnCaillou JeSuisUnCaillou deleted the ux/DAT-409/new-design-authorization-request-card branch August 28, 2024 08:57
@JeSuisUnCaillou
Copy link
Contributor

let's go ! :D

@skelz0r
Copy link
Member

skelz0r commented Aug 28, 2024

Jusqu'a présent dans le décorator, il y a surtout des methods helpers, je ne suis pas certaine que de déplacer une condition a une seule branche soit réellement un plus niveau lisibilité ?

Mais je suis d'accord qu'il y a peut être des questions à se poser quand à notre utilisation des decorators et ce que l'on doit y mettre ou pas ?

Tout ce qui est utilisé uniquement dans la vue et intrinséquement lié au modèle ça part dans le decorateur pour moi. Ensuite je fais confiance à nos règles de lint pour nous alerter dès que ça devient trop gros.

Copy link

@evaspae evaspae left a comment

Choose a reason for hiding this comment

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

C'est bon pour moi ! 👍

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.

5 participants