-
Notifications
You must be signed in to change notification settings - Fork 30
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
Ajout des pourcentages sur les données filtrées #299
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pour le nombre de rapport, je laisse l'auteur de l'issue décider si c'est suffisant ou pas.
@@ -25,7 +25,8 @@ public function execute() | |||
foreach ($this->queryBuilder->execute() as $row) { | |||
$slice = $row['salarySlice']; | |||
$results[$slice] = array( | |||
'count' => $row['nbResponse'] | |||
'count' => $row['nbResponse'], | |||
'nbResponse' => $row['nbResponse'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ne serait-il pas possible de mutualiser le count
et le nbResponse
? les deux contiennent la même information.
Plusieurs solutions je pense :
- soit renommer
count
ennbResponse
et modifier les fichiers utilisant lecount
- soit modifier la méthode
addPercentResponse
pour qu'elle prenne en paramètre facultatif le nom du champ à prendre en compte pour le calcul (par défaut nbResponse). Et dans tout cas, tu passe comme nomcount
.
Qu'en penses-tu ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oui tout à fait. Je n'ai pas voulu le faire dans cette PR et risquer de casser quelque chose que je n'aurai pas vu, mais l'intégrer dans une autre.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Si tu ne le fais pas maintenant tu ne le feras jamais je pense. Si tu as peur de la régression, tu peux très bien le faire dans un commit séparé ce qui permettrait de faire un revert facilement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK corrigé dans le dernier commit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Juste pour valider, ligne 36 à 38 on déclare un tableau baseResult
avec comme clé count
qui sert dans certains cas à remplir un autre tableau baseResults
. Cette clé count
était-elle fonctionnellement différente de la clé que tu as supprimée ?. Car dans certains cas, on fait un array_fill
avec ce baseResult
qu'on merge ensuite ligne 50 dans results
.
N'aurait-on donc pas dans certains cas un tableau results
qui contient pour certains clés un count
et dans d'autres cas un nbResponse
?
Pensez-vous intéressant de revoir la disposition des informations des tableaux par la même occasion ? Celà permettrait d'harmoniser un peu leur présentation. Ping @agallou |
@@ -25,7 +25,8 @@ public function execute() | |||
foreach ($this->queryBuilder->execute() as $row) { | |||
$slice = $row['salarySlice']; | |||
$results[$slice] = array( | |||
'count' => $row['nbResponse'] | |||
'count' => $row['nbResponse'], | |||
'nbResponse' => $row['nbResponse'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Juste pour valider, ligne 36 à 38 on déclare un tableau baseResult
avec comme clé count
qui sert dans certains cas à remplir un autre tableau baseResults
. Cette clé count
était-elle fonctionnellement différente de la clé que tu as supprimée ?. Car dans certains cas, on fait un array_fill
avec ce baseResult
qu'on merge ensuite ligne 50 dans results
.
N'aurait-on donc pas dans certains cas un tableau results
qui contient pour certains clés un count
et dans d'autres cas un nbResponse
?
@Spomky : concernant les tableaux, je pense que tu as raison. Par exemple, pour les colonnes qui contiennent des chiffres, parfois les résultats sont alignés à gauche, parfois ils sont alignés à droite (meilleur choix je pense). Il faudrait entrer une autre issue pour ça à mon avis. |
@@ -6,13 +6,15 @@ | |||
<tr> | |||
<th>{{ "report.view.remote_usage" | trans }}</th> | |||
<th>{{ "report.view.response_number" | trans }}</th> | |||
<th>{{ "report.view.response_percent" | trans }}</th> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tu parlais d'alignement, ça peux être intéressant d'aligner ces pourcentages à droite
Cette PR ajoute les pourcentages sur les données filtrées sur certaines pages du rapport.
Voir #294