UX/UI: Traitement par lot - Vue tableau des candidatures by hellodeloo · Pull Request #5089 · gip-inclusion/les-emplois

hellodeloo · 2024-11-12T16:06:55Z

🤔 Pourquoi ?

Indiquez le problème que nous sommes en train de résoudre et les objectifs métiers ou techniques qui sont visés par ces changements.

🍰 Comment ?

Décrivez en quelques mots la solution retenue et mise en oeuvre, les difficultés ou problèmes rencontrés. Attirez l'attention sur les décisions d'architecture ou de conception importantes.

🚨 À vérifier

Mettre à jour le CHANGELOG_breaking_changes.md ?

🏝️ Comment tester

Les instructions pour reproduire le problème, les profils de test, le parcours spécifique à utiliser, etc. Si vous disposez d'une recette jetable, mettre l'URL pour tester dans cette partie.

💻 Captures d'écran

notion-workspace · 2024-11-12T16:07:58Z

🌿 [1/2 Traitement par lot] Vue tableau des candidatures

github-actions · 2024-11-20T10:36:32Z

🥁 La recette jetable est prête ! 👉 Je veux tester cette PR !

francoisfreitag · 2024-12-03T14:43:51Z

+function querySelectorAllIncludingTarget(target, selector) {
+  const results = Array.from(target.querySelectorAll(selector))
+  if (target.matches(selector)) {
+    results.push(target)
+  }
+  return results
+}
+


Pourquoi déclarer cette fonction globalement ?

Je me demandais si on voulait pas généraliser son utilisation dans tout le fichier à la place d'un simple target.querySelectorAll.
Car quand on veut réinitialiser le JS sur une élément swappé et que cet élément swappé est à la racine (comme les boutons Liste/Tableau dans mon cas), c'est assez piégeux.

J’aurais tendance à dire non. querySelectorAll cherche les nœuds descendants, donc il me paraît clair qu’on n’aura pas target. Pour initialiser le JS sur l’élément swappé, il suffit de passer target à la fonction d’init à appeler, pas besoin de passer par un querySelectorAll.

L'idée c'est d'avoir le même JS qui gère les deux cas:

chargement d'une page de 0 (et l'élément à initialiser est bien un noeud descendant, donc querySelectorAll marche bien)

chargement lors d'un hx-swap-oob (et l'élément à initialiser est maintenant target donc querySelectorAll ne marche plus)

Il se trouve qu'actuellement nos éléments swappés sont souvent dans des div correspondant à une zone et donc le JS à initialiser n'est que rarement sur l'élément swappé.
Mais il suffit qu'un dev se dise qu'il est inutile de swapper le div et que swapper le form serait plus efficace (car ma foi, le div ne change pas en fait !): hop le JS censé initialiser le form ne fonctionnera plus.

Je vois l’idée, ça évitera sûrement des erreurs.

Je pense toujours que la fonction ne devrait pas être globale (mais déclarée dans le htmx.onLoad() ou une IIFE qui enregistre le htmx.onLoad()) ce qui était le but de mon commentaire initial.

Et du coup, plutôt pour une PR séparée qui promeut l’utilisation de querySelectorAllIncludingTarget dans ce module.

francoisfreitag · 2024-12-03T14:50:12Z

+  document.querySelectorAll('[data-it-tr-href] > td').forEach((tableCol) => {
+      tableCol.addEventListener('click', function() {
+          window.open(this.closest('[data-it-tr-href]').dataset.itTrHref, '_self');
+      })
+  })


Je ne suis vraiment pas fan de cette idée. Lorsqu’on aura des contrôles interactifs dans les <td>, on aura un conflit entre le click sur le contrôle et le clic sur le <tr>. Et le clic qques pixels à côté qui ouvre la page de détail de la candidature et perd la sélection.

À mon avis, mieux vaudrait utiliser la date d’émission comme un lien vers la candidature (ou bouger le bouton Gérer / le rendre plus facilement accessible)

Yup, les lignes cliquables dans les tableaux, le HTML n'a pas été fait pour 😬
Mais je crois que le métier y tient et tant qu'on ne rajoute rien de cliquable dans les lignes, ça devrait bien se passer 🤞 ?

On ne va pas rajouter d’ici quelques jours un <input type="checkbox"> ?

Pour moi, on dit devrait expliquer au métier que ce n’est pas possible techniquement (on ne veut pas avoir à deviner si l’utilisateur voulait cliquer sur la ligne ou le contrôle) et pas une bonne idée. Plusieurs conventions assez universelles du web et d’accessibilité (lien qui s’affiche au survol de la souris, éléments interactifs mis en surbrillance/focusable) sont bafouées.

@francoisfreitag J'ai déjà argumenté mon possible, mais sans succès 😔
Pour le <th scope="row"><input type="checkbox"></th>, il sera cliquable séparément de la ligne, c'est déjà prévu par le css/js

On ne devrait surtout pas utiliser l’implémentation actuelle, et éventuellement revoir le design pour faciliter l’implémentation:

L’accessibilité. Rien ne laisse imaginer ce qu’il se passe lorsqu’on clique sur une ligne. Le navigateur ne peut pas donner l’info que c’est un lien, car nous n’avons pas construit de lien. Aussi, le label pour l’élément n’est pas clair. S’agit-il de la concaténation de tous les textes de la ligne ? En essayant d’inspecter l’accessibilité dans mon navigateur, je vois la violation suivante : clickable elements should be focusable and should have interactive semantics, voir screenshot ci-dessous. Voir le thread mattermost sur domaine-accessibilité de beta pour une même fonctionnalité : https://mattermost.incubateur.net/betagouv/pl/1tr6i8uh5idxmjjtjybyqt1jro

La sémantique (déguiser un lien empêche les utilisateurs de reconnaître et bénéficier des patterns bien établis sur le web en général, le survol avec la souris n’indique pas l’endroit où l’utilisateur sera redirigé)

les modifieurs fréquemment utilisés sur le web ne marchent pas (clic de la souris pour ouvrir dans un nouvel onglet, ctrl+clic, shift+clic, etc.). De la même manière, les plugins navigateurs permettant d’intéragir avec les liens ne fonctionneront pas.

Une implémentation alternative potentiellement acceptable me semblerait d’étendre le lien sur la ligne via un stretched-link. Autrement, peut-on éventuellement imaginer rendre la date cliquable au lieu de la ligne ? On pourrait ainsi construire un lien avec toutes les propriétés qui vont bien, éventuellement l’étendre à la cellule (<td>). Il serait facile d’accès aux utilisateurs (premier élément tant qu’il n’y a pas la checkbox, second ensuite). En bout de ligne, on peut conserver le bouton gérer, de manière à ce que les utilisateurs gardent sous la main un moyen d’accès à la candidature. J’imagine qu’on pourrait aussi figer la première (les deux premières) colonnes si nécessaire et retirer le lien gérer.

francoisfreitag · 2024-12-03T15:03:48Z

+                Candidature spontanée
+            {% else %}
+                <ul class="list-unstyled">
+                    {% for job in all_jobs %}<li>{{ job.display_name }}</li>{% endfor %}


J’imagine qu’on aura besoin d’un collapse ici à un moment ?

francoisfreitag · 2024-12-03T15:12:42Z

+    for display_param in [
+        {},
+        {"display": JobApplicationsDisplayKind.LIST},
+        {"display": JobApplicationsDisplayKind.TABLE},
+    ]:


À mon avis, un test dédié suffirait. Les filtres étant indépendant, on pourrait tester toutes les combinaisons de cette manière, ce qui serait un beau bazar et prendrait des plombes.

En revanche, j’ajouterais bien le cas {"display": "invalid"} au test spécifique.

J'ai hésité et bonne idée pour le {"display": "invalid"}. Par contre, comme je ne pense pas que vérifier la présence d'un <caption class="visually-hidden">Liste des candidatures</caption> ou d'un <div class="c-box c-box--results has-links-inside my-3 my-md-4"> soit un super test, je vais juste intégrer cela dans le test de snapshot.

Je voulais dire d’écrire un test qui vérifie le contenu de la page sans filtres avec des candidatures, en faisant display=list et display=table. Plutôt que de se brancher sur des tests existants qui vérifient le fonctionnement d’autres filtres et de les rendre plus complexes et plus lents.

francoisfreitag · 2024-12-03T15:15:24Z

Autrement, ça marche bien dans mes tests 👍

francoisfreitag

#5089 (comment)

francoisfreitag · 2024-12-09T16:46:21Z

+function querySelectorAllIncludingTarget(target, selector) {
+  const results = Array.from(target.querySelectorAll(selector))
+  if (target.matches(selector)) {
+    results.push(target)
+  }
+  return results
+}
+


À ne pas rater au rebase.

Bien vu, je l'avais effectivement raté 😬

francoisfreitag

Super 👍

hellodeloo self-assigned this Nov 12, 2024

hellodeloo marked this pull request as draft November 12, 2024 16:07

hellodeloo changed the title ~~wip: add batch processing for applications~~ UX/UI: Traitement par lot - Vue tableau des candidatures Nov 12, 2024

hellodeloo assigned xavfernandez Nov 12, 2024

hellodeloo added the modifié Modifié dans le changelog. label Nov 12, 2024

hellodeloo force-pushed the deloo/feat-batch-processing-of-applications branch 2 times, most recently from dd5724d to 645155d Compare November 14, 2024 10:09

xavfernandez force-pushed the deloo/feat-batch-processing-of-applications branch 2 times, most recently from 1fa8b92 to 8d84463 Compare November 20, 2024 10:25

xavfernandez added the 1-recette-jetable [Payé à l’heure] Crée une recette jetable sur CC label Nov 20, 2024

xavfernandez force-pushed the deloo/feat-batch-processing-of-applications branch 3 times, most recently from f99bd34 to e8dbc8f Compare November 20, 2024 14:35

xavfernandez mentioned this pull request Nov 20, 2024

Candidature: correction de la couleur du badge dans le cas de l'Éligibilité IAE à valider #5133

Merged

1 task

xavfernandez force-pushed the deloo/feat-batch-processing-of-applications branch 13 times, most recently from 3b0a26e to 1465ca3 Compare November 25, 2024 09:23

hellodeloo force-pushed the deloo/feat-batch-processing-of-applications branch from f62ae4c to 4733739 Compare November 25, 2024 16:24

xavfernandez force-pushed the deloo/feat-batch-processing-of-applications branch 10 times, most recently from cc0af33 to 23a9573 Compare November 29, 2024 12:17

xavfernandez marked this pull request as ready for review November 29, 2024 12:20

xavfernandez force-pushed the deloo/feat-batch-processing-of-applications branch 3 times, most recently from 2f0defc to f918013 Compare November 29, 2024 14:04

xavfernandez requested review from francoisfreitag and rsebille December 2, 2024 09:11

xavfernandez force-pushed the deloo/feat-batch-processing-of-applications branch 2 times, most recently from a2daffc to e83f460 Compare December 2, 2024 21:52

francoisfreitag reviewed Dec 3, 2024

View reviewed changes

xavfernandez force-pushed the deloo/feat-batch-processing-of-applications branch 3 times, most recently from 4b8faac to 6ec56d1 Compare December 3, 2024 16:48

francoisfreitag requested changes Dec 4, 2024

View reviewed changes

xavfernandez force-pushed the deloo/feat-batch-processing-of-applications branch 3 times, most recently from 6349ddb to e728af7 Compare December 9, 2024 08:00

francoisfreitag reviewed Dec 9, 2024

View reviewed changes

francoisfreitag approved these changes Dec 9, 2024

View reviewed changes

Comment thread tests/www/apply/test_list_for_siae.py Outdated

apply: add table view for applications

412d42f

Conversation

hellodeloo commented Nov 12, 2024

🤔 Pourquoi ?

🍰 Comment ?

🚨 À vérifier

🏝️ Comment tester

💻 Captures d'écran

Uh oh!

notion-workspace Bot commented Nov 12, 2024

Uh oh!

github-actions Bot commented Nov 20, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisfreitag Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hellodeloo Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisfreitag commented Dec 3, 2024

Uh oh!

francoisfreitag left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisfreitag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

francoisfreitag Dec 3, 2024 •

edited

Loading

hellodeloo Dec 3, 2024 •

edited

Loading