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

[Simplification des plages d'ouvertures] Amélioration du formulaire #4328

Merged
merged 8 commits into from
Jun 5, 2024

Conversation

ousmanedev
Copy link
Contributor

@ousmanedev ousmanedev commented Jun 4, 2024

Simplification des plages d'ouverture

Amélioration du formulaire

Cette PR propose une première itération du projet des plages d'ouverture.
Les changements ici, se focalisent uniquement sur quelques améliorations sur le formulaire de création des plages d'ouverture, notamment:

  • Remplacer "Description" par "Nom de la plage d'ouverture", avec un nouveau placeholder et suppression du hint
  • Afficher le champ Lieu, uniquement quand un motif sur place, est sélectionné
  • Séparer en 2 cards, les sections haute (informations de base) et basse du formulaire (informations de récurrence)
  • Changer le nom du bouton de soumission du formulaire, de "Enregistrer" à "Créer la plage d'ouverture

Screenshots

Description

Avant Après
1 - before 1 - after

Lieu

Désormais, le champ de sélection du Lieu ne s'affiche que quand on coche un motif sur place

Avant Après
2 - before 2 - after

Sections du formulaire

Le formulaire est séparé en 2 sections (ou cards)

Avant Après
4 - before 4 - after

Bouton de soumission

"Enregistrer" devient "Créer la plage d'ouverture

Avant Après
3 - before 3 - after

Pour plus d'informations :

@ousmanedev ousmanedev changed the title [Plage d'ouvertures] Ameliorations du formulaire [Simplification des plages d'ouvertures] Ameliorations du formulaire Jun 4, 2024
@ousmanedev ousmanedev marked this pull request as draft June 4, 2024 13:55
@ousmanedev ousmanedev changed the title [Simplification des plages d'ouvertures] Ameliorations du formulaire [Simplification des plages d'ouvertures] Amélioration du formulaire Jun 4, 2024
@ousmanedev ousmanedev marked this pull request as ready for review June 4, 2024 16:07
@ousmanedev ousmanedev force-pushed the po-v0-0 branch 2 times, most recently from 0e155d6 to 77874bf Compare June 4, 2024 17:54
Copy link
Contributor

@adipasquale adipasquale left a comment

Choose a reason for hiding this comment

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

ça marche nickel 👍 et ce sont de très bonnes améliorations d’UX

J’ai mis quelques commentaires non-bloquants mais si tu as le courage de corriger celui du formulaire qui se déplie au chargement de la page ça serait une nette amélioration !

constructor() {
this.toggleLieuSelectionField();

let that = this;
Copy link
Contributor

Choose a reason for hiding this comment

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

ce n’est pas nécessaire il me semble, la fat arrow () => { préserve déjà le this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merci. Corrigé dans 68d0349

@@ -0,0 +1,21 @@
class PlageOuverture {
constructor() {
this.toggleLieuSelectionField();
Copy link
Contributor

@adipasquale adipasquale Jun 5, 2024

Choose a reason for hiding this comment

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

un petit bémol à cette manière de faire pour l’état initial c’est que si un motif est déjà coché lors du chargement du dom ça va faire un "layout shift" avec le uncollapse en différé

deux manière de reproduire ça c’est de :

  1. sur le formulaire new faire volontairement une erreur de validation, par ex mettre une heure de fin antérieure à l’heure de début
  2. aller sur le formulaire edit d’ue PO avec un lieu sur place

pour réparer ça il faudrait supprimer cette ligne et faire en sorte que le DOM soit chargé correctement avec les bons attributs de collapse et de show dès le render server
ça demande un peu plus de taf et un peu de duplication de la logique côté serveur et client mais ça vaut le coût à mon avis !

Screen.Recording.2024-06-05.at.10.49.20.mov

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. J'ai trouvé une manière de corriger ça côté client, en supprimant la transition sur le collapse, lors du premier render. Voir 4ccddd2

Copy link
Contributor

Choose a reason for hiding this comment

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

bien vu c’est une bonne manière d’améliorer à bas-coût ! Il reste un petit délai entre le chargement du DOM et l’exécution du JS, invisible sur un réseau rapide et un PC performant, mais c’est probablement négligeable 👍

}

toggleLieuSelectionField() {
let selectedMotifsPublicOffice = $(".plage-ouverture-form .form-check-input.public_office[name='plage_ouverture[motif_ids][]']:checked");
Copy link
Contributor

Choose a reason for hiding this comment

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

tu peux utiliser un const ici

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merci. Fait ici 68d0349

.pl-3
= f.check_box :motif_ids, { multiple: true, class: "form-check-input #{motif.location_type}" }, motif.id, false
= f.label "motif_ids_#{motif.id}", motif_name_with_location_type_and_badges(motif), class: "form-check-label"
.collapse.lieu-field
Copy link
Contributor

Choose a reason for hiding this comment

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

je ne pense pas qu’il y ait de doctrine claire chez RDV mais perso j’aime bien préfixer les classes utilisées uniquement pour le js : js-lieu-field

Copy link
Contributor

Choose a reason for hiding this comment

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

je suis de la même école, et sinon une alternative que je trouve pas mal c'est d'utiliser des data attributes pour clarifier que c'est destiné à du js plutôt que du css

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fait ici 68d0349

@@ -7,19 +7,4 @@
- else
| Nouvelle plage d'ouverture pour #{@plage_ouverture.agent.full_name}

- content_for :breadcrumb do
Copy link
Contributor

Choose a reason for hiding this comment

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

pourquoi avoir supprimé les breadcrumbs ici ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Il m'a semblé dans un premier temps que le nouveau design le supprimait, mais je me suis trompé.

Restauré ici : ece0fff

@@ -4,7 +4,7 @@ fr:
plage_ouverture: Plage d'ouverture
attributes:
plage_ouverture:
title: Description
title: Nom de la plage d'ouverture
Copy link
Contributor

Choose a reason for hiding this comment

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

❤️ tellement mieux

@ousmanedev
Copy link
Contributor Author

Merci pour la review @adipasquale & @victormours

J'ai répondu à l'ensemble des commentaires et appliqué les correctifs.

@ousmanedev ousmanedev enabled auto-merge (rebase) June 5, 2024 11:58
@ousmanedev ousmanedev merged commit ed93e0e into production Jun 5, 2024
11 of 12 checks passed
@ousmanedev ousmanedev deleted the po-v0-0 branch June 5, 2024 11:58
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.

None yet

3 participants