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

ETQ admin : je peux ordonner les labels #11237

Merged
merged 6 commits into from
Jan 28, 2025

Conversation

Benoit-MINT
Copy link
Contributor

@Benoit-MINT Benoit-MINT commented Jan 22, 2025

issue: #11116

Résultat :
1- L'admin peut ordonner les labels :
Capture d’écran 2025-01-22 à 16 27 43

2- Les instructeurs ont ainsi le même ordre dans :

  • le filtre "labels" ;
  • la colonne "labels" sur le tableau des dossiers ;
  • la liste des labels déjà associés dans la vue du dossier ;
  • la liste des labels pouvant être associés dans la vue dossier ;
  • l'export des dossiers, colonne labels.

Capture d’écran 2025-01-22 à 16 32 55
Capture d’écran 2025-01-22 à 16 34 42
Capture d’écran 2025-01-22 à 16 35 22
Capture d’écran 2025-01-22 à 16 36 22

@Benoit-MINT Benoit-MINT force-pushed the etq-administrateur-ordonner-labels branch from cf0566f to ae8fbc6 Compare January 22, 2025 16:17
Copy link

codecov bot commented Jan 22, 2025

Codecov Report

Attention: Patch coverage is 66.66667% with 5 lines in your changes missing coverage. Please review.

Project coverage is 83.91%. Comparing base (fc86396) to head (307b333).
Report is 11 commits behind head on main.

Files with missing lines Patch % Lines
...p/controllers/administrateurs/labels_controller.rb 37.50% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #11237      +/-   ##
==========================================
- Coverage   84.41%   83.91%   -0.51%     
==========================================
  Files        1202     1202              
  Lines       26416    26433      +17     
  Branches     4974     4974              
==========================================
- Hits        22298    22180     -118     
- Misses       4118     4253     +135     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Benoit-MINT Benoit-MINT force-pushed the etq-administrateur-ordonner-labels branch 2 times, most recently from cbdf9fa to f7e42af Compare January 22, 2025 16:32
@Benoit-MINT Benoit-MINT marked this pull request as ready for review January 22, 2025 16:41
@colinux colinux mentioned this pull request Jan 23, 2025
7 tasks
Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

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

J'ai une contre proposition:

dans procedure.rb, tu modifies la relation has_many :label ... par has_many :label, -> { order(:position, :id) } ...

De cette façon, les labels seront toujours ordonnés de la mm manière mm si aucune position n'est renseignée.

Du coup, tu devrais pouvoir shooter tout le code qui remplit une position par défaut et aussi les labels.order(:position) disséminés dans le code.

WDYT ?

@Benoit-MINT Benoit-MINT force-pushed the etq-administrateur-ordonner-labels branch from f7e42af to e389a19 Compare January 27, 2025 15:20
@Benoit-MINT
Copy link
Contributor Author

Benoit-MINT commented Jan 27, 2025

J'ai une contre proposition:

dans procedure.rb, tu modifies la relation has_many :label ... par has_many :label, -> { order(:position, :id) } ...

De cette façon, les labels seront toujours ordonnés de la mm manière mm si aucune position n'est renseignée.

Du coup, tu devrais pouvoir shooter tout le code qui remplit une position par défaut et aussi les labels.order(:position) disséminés dans le code.

WDYT ?

yes, bonne idée! merci ;)
j'ai pu supprimer en effet le code qui définit une position par défaut.
J'ai répliqué par ailleurs ton idée dans le model dossier, ce qui a permis de simplifier dans les cas où l'on appelait labels sur dossier. En revanche pour la cas où on appelle depuis la table DossierLabel, j'ai conservé la proposition initiale.

@Benoit-MINT Benoit-MINT force-pushed the etq-administrateur-ordonner-labels branch from e389a19 to 737f536 Compare January 27, 2025 15:52
Copy link
Member

@LeSim LeSim left a comment

Choose a reason for hiding this comment

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

yes !

@Benoit-MINT Benoit-MINT force-pushed the etq-administrateur-ordonner-labels branch from 737f536 to 307b333 Compare January 28, 2025 15:01
@Benoit-MINT Benoit-MINT added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
@Benoit-MINT Benoit-MINT added this pull request to the merge queue Jan 28, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 28, 2025
@Benoit-MINT Benoit-MINT added this pull request to the merge queue Jan 28, 2025
Merged via the queue into main with commit 41a0da0 Jan 28, 2025
19 checks passed
@Benoit-MINT Benoit-MINT deleted the etq-administrateur-ordonner-labels branch January 28, 2025 15:47
.fr-table.fr-table--lg
.fr-table__wrapper
.fr-table__container
.fr-table__content
%table
%caption Liste des labels
Copy link
Member

Choose a reason for hiding this comment

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

J'arrive après la bataille mais c'est dommage de retirer la caption en faveur d'un titre ! (on devrait en avoir sur toutes nos tables, en tout cas dès qu'on a une exigence en accessibilité, ce qui certes est un peu plus discutable côté admin)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep ... j'ai eu la discussion avec Marlène et Lisa.
En fait, la contrainte d'avoir le bouton "personnaliser" aligné horizontalement avec le titre initialement dans caption n'était pas compatible (j'ai tenté plusieurs astuces en vain) sur l'idée de disposer en justify-between. Il y a un comportement JS propre à caption qui ramène à gauche tout ce qui se trouve dans cette balise, malgré les class qui vont bien.
Et en effet toutes les autres tables qui ont dans l'esprit plusieurs éléments là où l'on a le titre du tableau, n'ont pas/plus de balise caption.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dans le cas ici, et d'autres tableaux "simples", sous couvert de Marlène, on pourrait avoir un équilibre access/UX en acceptant de décaler verticalement. En revanche pour des tableaux plus complexes, comme le suivi instructeur des dossiers, ça paraît plus complexe

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.

3 participants