entrouvert
/
combo
16
1
Fork 0
Code Issues Pull Requests 11 Releases Wiki Activity

dataviz: adapt chart's height to content (#62282) #54

Merged
smihai merged 4 commits from wip/62282-dataviz-allow-charts-auto-height into main 2023-03-27 11:43:58 +02:00
Conversation 8 Commits 4 Files Changed 3 +180 -22
smihai commented 2023-02-23 17:04:04 +01:00
Owner
Copy Link

Proposition donc d'ajouter une option "Automatic" pour la hauteur qui ajustera la hauteur du SVG produit s'il y a des libellés sur l'axe Y des graphes.

Je ne modifie pas la valeur par défaut pour la hauteur car elle semble plus logique dans la plupart des cas.

Proposition donc d'ajouter une option "Automatic" pour la hauteur qui ajustera la hauteur du SVG produit s'il y a des libellés sur l'axe Y des graphes. Je ne modifie pas la valeur par défaut pour la hauteur car elle semble plus logique dans la plupart des cas.
Accueil Portail Agent.png
90 KiB
Accueil Portail Agent.png
fpeters commented 2023-02-23 17:30:44 +01:00
Owner
Copy Link

Pas fan de "automatique" qui peut apparaitre vague, plutôt, si on parle bien de ça "Adaptée selon la légende" ?

(sur les captures on voit que le nombre du haut de l'axe (100 ou 2) est tronqué, je ne sais pas si ça vient de ce patch).

Pas fan de "automatique" qui peut apparaitre vague, plutôt, si on parle bien de ça "Adaptée selon la légende" ? (sur les captures on voit que le nombre du haut de l'axe (100 ou 2) est tronqué, je ne sais pas si ça vient de ce patch).
vdeniaud commented 2023-02-23 17:49:30 +01:00
Owner
Copy Link

Ça fait un graphe plutôt ratatiné si il n'y a pas beaucoup de légendes, donc éventuellement garder une hauteur minimale (en prenant le réglage de hauteur « Moyenne » ou « Petite »).

Ça fait un graphe plutôt ratatiné si il n'y a pas beaucoup de légendes, donc éventuellement garder une hauteur minimale (en prenant le réglage de hauteur « Moyenne » ou « Petite »).
vdeniaud commented 2023-02-23 18:09:24 +01:00
Owner
Copy Link

Aussi comme le notait Thomas dans le ticket lié il y a un endroit où on fait chart.legend_at_bottom = True, ce qui à mon avis devrait désactiver le calcul automatique et revenir à la hauteur « par défaut ».

Aussi comme le notait Thomas dans le ticket lié il y a un endroit où on fait `chart.legend_at_bottom = True`, ce qui à mon avis devrait désactiver le calcul automatique et revenir à la hauteur « par défaut ».
smihai commented 2023-02-24 11:15:01 +01:00
Author
Owner
Copy Link

Pas fan de "automatique" qui peut apparaitre vague, plutôt, si on parle bien de ça "Adaptée selon la légende" ?

Comme la légende n'est pas toujours affichée, typiquement quand il n'y a pas de regroupement défini j'ai préféré ne pas la mentionner.

(sur les captures on voit que le nombre du haut de l'axe (100 ou 2) est tronqué, je ne sais pas si ça vient de ce patch).

En effet, je vais creuser ça.

> Pas fan de "automatique" qui peut apparaitre vague, plutôt, si on parle bien de ça "Adaptée selon la légende" ? > Comme la légende n'est pas toujours affichée, typiquement quand il n'y a pas de regroupement défini j'ai préféré ne pas la mentionner. > (sur les captures on voit que le nombre du haut de l'axe (100 ou 2) est tronqué, je ne sais pas si ça vient de ce patch). En effet, je vais creuser ça.
smihai changed title from dataviz: allow chart's auto height (#62282) to WIP: dataviz: allow chart's auto height (#62282) 2023-02-24 11:15:05 +01:00
fpeters commented 2023-02-24 11:17:46 +01:00
Owner
Copy Link

Comme la légende n'est pas toujours affichée, typiquement quand il n'y a pas de regroupement défini j'ai préféré ne pas la mentionner.

Je reste peu enthousiaste sur le "automatique"; combiné au commentaire de Valentin on pourrait peut-être convenir que le paramétrage actuel suffit mais que le comportement devrait être d'étendre pour pouvoir contenir la légende, quand nécessaire. (parce que vraiment il y a peu de sens à choisir "petit et affiche un quart de légende").

> Comme la légende n'est pas toujours affichée, typiquement quand il n'y a pas de regroupement défini j'ai préféré ne pas la mentionner. Je reste peu enthousiaste sur le "automatique"; combiné au commentaire de Valentin on pourrait peut-être convenir que le paramétrage actuel suffit mais que le comportement devrait être d'étendre pour pouvoir contenir la légende, quand nécessaire. (parce que vraiment il y a peu de sens à choisir "petit et affiche un quart de légende").
smihai commented 2023-02-24 11:44:26 +01:00
Author
Owner
Copy Link

Je reste peu enthousiaste sur le "automatique"; combiné au commentaire de Valentin on pourrait peut-être convenir que le paramétrage actuel suffit mais que le comportement devrait être d'étendre pour pouvoir contenir la légende, quand nécessaire. (parce que vraiment il y a peu de sens à choisir "petit et affiche un quart de légende").

Ok, je vais juste ajuster la hauteur pour afficher la légende en entier quand nécessaire.

> > Je reste peu enthousiaste sur le "automatique"; combiné au commentaire de Valentin on pourrait peut-être convenir que le paramétrage actuel suffit mais que le comportement devrait être d'étendre pour pouvoir contenir la légende, quand nécessaire. (parce que vraiment il y a peu de sens à choisir "petit et affiche un quart de légende"). > Ok, je vais juste ajuster la hauteur pour afficher la légende en entier quand nécessaire.
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from f00c1e6f6d to bf3a749c24 2023-02-27 10:05:16 +01:00 Compare
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from bf3a749c24 to 7ffc8985b6 2023-02-27 16:11:19 +01:00 Compare
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from 7ffc8985b6 to 1a36949bbc 2023-02-27 16:12:35 +01:00 Compare
smihai changed title from WIP: dataviz: allow chart's auto height (#62282) to dataviz: adapt chart's height to content (#62282) 2023-02-27 16:12:44 +01:00
vdeniaud reviewed 2023-02-27 16:35:13 +01:00
combo/apps/dataviz/models.py Outdated
@ -351,6 +352,8 @@ class ChartNgCell(CellBase):
if self.statistic.service_slug == 'bijoe':
x_labels, y_labels, data = self.parse_response(response, chart)
chart.x_labels = x_labels
height = max(int(self.height), auto_height_scale * len(y_labels))
vdeniaud commented 2023-02-27 16:35:13 +01:00
Owner
Copy Link

À mon avis ici et plus bas il faut utiliser le height passé à la méthode et pas le self.height (pour ne pas casser la fonctionnalité « hauteur passée explicitement » dont on s'aperçoit qu'elle n'est pas testée).

À mon avis ici et plus bas il faut utiliser le `height` passé à la méthode et pas le `self.height` (pour ne pas casser la fonctionnalité « hauteur passée explicitement » dont on s'aperçoit qu'elle n'est pas testée).
smihai commented 2023-02-27 16:57:18 +01:00
Author
Owner
Copy Link

height peut être None (on le voit dans les tests) et c'est pour ça que j'ai préféré utiliser self.height qui de toute façon est pris en compte dans la vue:

        try:
            chart = form.instance.get_chart(
                width=int(request.GET['width']) if request.GET.get('width') else None,
                height=int(request.GET['height']) if request.GET.get('height') else int(self.cell.height),
            )
`height` peut être `None` (on le voit dans les tests) et c'est pour ça que j'ai préféré utiliser `self.height` qui de toute façon est pris en compte dans la vue: ``` try: chart = form.instance.get_chart( width=int(request.GET['width']) if request.GET.get('width') else None, height=int(request.GET['height']) if request.GET.get('height') else int(self.cell.height), ) ```
vdeniaud commented 2023-02-27 17:18:01 +01:00
Owner
Copy Link

on le voit dans les tests

Ouep c'est une situation qui semble n'exister que dans les tests, c'est assez nul mais bon, autre combat !

> on le voit dans les tests Ouep c'est une situation qui semble n'exister que dans les tests, c'est assez nul mais bon, autre combat !
vdeniaud reviewed 2023-02-27 16:35:54 +01:00
combo/apps/dataviz/models.py Outdated
@ -555,6 +561,7 @@ class ChartNgCell(CellBase):
if self.chart_type != 'pie':
if width and width < 500:
chart.config.height = int(self.height)
vdeniaud commented 2023-02-27 16:35:54 +01:00
Owner
Copy Link

À quoi sert cette ligne ?

À quoi sert cette ligne ?
smihai commented 2023-02-27 16:58:45 +01:00
Author
Owner
Copy Link

A garder la hauteur définie de la cellule lorsque

chart.legend_at_bottom = True
A garder la hauteur définie de la cellule lorsque ``` chart.legend_at_bottom = True ```
vdeniaud commented 2023-02-27 17:15:02 +01:00
Owner
Copy Link

Ah oui désolé j'aurais pu m'en apercevoir... Ça serait bien d'avoir un test si tu te sens.

Et c'est aussi un endroit où on perd la valeur du paramètre height passé dans la requête, il ne faudrait pas.

Ah oui désolé j'aurais pu m'en apercevoir... Ça serait bien d'avoir un test si tu te sens. Et c'est aussi un endroit où on perd la valeur du paramètre `height` passé dans la requête, il ne faudrait pas.
vdeniaud commented 2023-02-28 10:04:05 +01:00
Owner
Copy Link

Cool je vois le nouveau test, par contre la remarque sur conserver le paramètre height me paraît toujours importante (tu peux voir dans le /manage/ que ça semi-casse le rendu du graphe).

Cool je vois le nouveau test, par contre la remarque sur conserver le paramètre `height` me paraît toujours importante (tu peux voir dans le `/manage/` que ça semi-casse le rendu du graphe).
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from 1a36949bbc to bf687b5923 2023-02-27 17:00:43 +01:00 Compare
vdeniaud reviewed 2023-02-27 17:16:29 +01:00
combo/apps/dataviz/models.py Outdated
@ -359,6 +362,9 @@ class ChartNgCell(CellBase):
else:
data = response['data']
if len(data['series']) > 1:
vdeniaud commented 2023-02-27 17:16:29 +01:00
Owner
Copy Link

Depuis les nouvelles versions du patch cette condition semble superflue.

Depuis les nouvelles versions du patch cette condition semble superflue.
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from bf687b5923 to 15d8ed2402 2023-02-27 18:58:31 +01:00 Compare
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from 15d8ed2402 to 33e9435f6a 2023-03-09 14:43:53 +01:00 Compare
smihai changed title from dataviz: adapt chart's height to content (#62282) to WIP: dataviz: adapt chart's height to content (#62282) 2023-03-09 14:44:35 +01:00
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from 33e9435f6a to 979474121b 2023-03-13 16:39:11 +01:00 Compare
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from 979474121b to 4e70f77d6d 2023-03-13 17:07:39 +01:00 Compare
smihai changed title from WIP: dataviz: adapt chart's height to content (#62282) to dataviz: adapt chart's height to content (#62282) 2023-03-13 17:14:25 +01:00
smihai commented 2023-03-13 17:16:26 +01:00
Author
Owner
Copy Link

J'ai un peu reorganisé le code car j'avais du mal à m'y trouver.

J'ai un peu reorganisé le code car j'avais du mal à m'y trouver.
vdeniaud requested changes 2023-03-15 10:00:56 +01:00
vdeniaud left a comment
Owner
Copy Link

Pas fan de 0002, pour moi ça ne simplifie pas la lecture du code :)

Sur le fond le bug que je pointais n'est pas corrigé.

Aussi un pprint ajouté dans un commit puis retiré (je le pointe ici car je ne peux pas commenter la ligne sur gitea, vu qu'elle n'apparaît pas dans le diff unifié...)

Pas fan de 0002, pour moi ça ne simplifie pas la lecture du code :) Sur le fond le bug que je pointais n'est pas corrigé. Aussi un pprint ajouté dans un commit puis retiré (je le pointe ici car je ne peux pas commenter la ligne sur gitea, vu qu'elle n'apparaît pas dans le diff unifié...)
combo/apps/dataviz/models.py
@ -597,7 +606,6 @@ class ChartNgCell(CellBase):
return new_data
def sort_values(self, chart, data):
vdeniaud commented 2023-03-15 09:58:11 +01:00
Owner
Copy Link

Changement superflu.

Changement superflu.
combo/apps/dataviz/models.py
@ -535,0 +534,4 @@
# use a single colour for dots
chart.config.style.colors = ('#1f77b4',) * max(len(chart.x_labels), 1)
def configure_horizontal_bar_chart(self, chart, *args):
vdeniaud commented 2023-03-15 09:32:49 +01:00
Owner
Copy Link

Il faut garder la condition width < 500, le fait que ça passe sans traduit juste un manque de test.

Il faut garder la condition `width < 500`, le fait que ça passe sans traduit juste un manque de test.
smihai commented 2023-03-16 17:47:22 +01:00
Author
Owner
Copy Link

Bien vu.
J'ai remis la condition width < 500 et rajouté un test.

Bien vu. J'ai remis la condition `width < 500` et rajouté un test.
🚀 1
combo/apps/dataviz/models.py
@ -537,3 +555,2 @@
chart.config.width = width
if height:
chart.config.height = height
if not height:
vdeniaud commented 2023-03-15 09:40:24 +01:00
Owner
Copy Link

J'aime bien ce changement, par soucis de cohérence ça serait cool d'avoir la même chose pour width genre juste en remplaçant le bloc if width: du dessus par width = width or int(self.width) (en plus de la modif à DatavizGraphView). Idéalement ce changement irait dans un commit séparé, mais pas très important.

J'aime bien ce changement, par soucis de cohérence ça serait cool d'avoir la même chose pour width genre juste en remplaçant le bloc `if width:` du dessus par `width = width or int(self.width)` (en plus de la modif à DatavizGraphView). Idéalement ce changement irait dans un commit séparé, mais pas très important.
smihai commented 2023-03-16 17:48:08 +01:00
Author
Owner
Copy Link

Fait un commit séparé pour height. En revanche self.width n'existe pas.

Fait un commit séparé pour `height`. En revanche `self.width` n'existe pas.
vdeniaud commented 2023-03-16 17:53:53 +01:00
Owner
Copy Link

Ah oui, désolé pour l'embrouille

Ah oui, désolé pour l'embrouille
combo/apps/dataviz/models.py
@ -564,3 +583,4 @@
if self.chart_type != 'pie':
if width and width < 500:
height = self.height
vdeniaud commented 2023-03-15 09:57:52 +01:00
Owner
Copy Link

C'est sur cette ligne que portait mon unique réserve sur le patch précédent, je ne sais pas pourquoi elle est encore là ?

En plus avant c'était chart.config.height = int(self.height), maintenant le chart.config a disparu donc la ligne ne fait plus rien.

Encore une fois le bug c'est qu'en rendu BO ça va produire un graphe trop haut. Il faut que cette ligne restaure la valeur originale de height avant correction (self.height ou la valeur transmise en paramètre, selon les cas). Ça serait bien aussi d'ajouter un test là dessus.

C'est sur cette ligne que portait mon unique réserve sur le patch précédent, je ne sais pas pourquoi elle est encore là ? En plus avant c'était `chart.config.height = int(self.height)`, maintenant le `chart.config` a disparu donc la ligne ne fait plus rien. Encore une fois le bug c'est qu'en rendu BO ça va produire un graphe trop haut. Il faut que cette ligne restaure la valeur originale de height avant correction (self.height ou la valeur transmise en paramètre, selon les cas). Ça serait bien aussi d'ajouter un test là dessus.
smihai commented 2023-03-16 17:49:17 +01:00
Author
Owner
Copy Link

Je me plantais à chaque fois ici, fatigue.

Je me plantais à chaque fois ici, fatigue.
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from 4e70f77d6d to d3e98b1a42 2023-03-16 17:47:14 +01:00 Compare
smihai requested review from vdeniaud 2023-03-16 17:49:22 +01:00
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from d3e98b1a42 to 7490f2c1cb 2023-03-16 17:50:55 +01:00 Compare
vdeniaud reviewed 2023-03-16 17:56:55 +01:00
combo/apps/dataviz/models.py
@ -542,0 +562,4 @@
chart.legend_at_bottom = True
else:
# adapt chart's height to legend length
height = max(height, auto_height_scale * len(chart.raw_series))
vdeniaud commented 2023-03-16 17:56:55 +01:00
Owner
Copy Link

Mais le graphe de type pie peut également voir sa légende déborder en hauteur, non ? (peut-être que je me plante)

Si c'est le cas il doit bénéficier de la correction du patch, et donc ce serait plutôt se placer sous ce bloc et faire

if not chart.legend_at_bottom:
    # adapt height
    height = max(...
Mais le graphe de type pie peut également voir sa légende déborder en hauteur, non ? (peut-être que je me plante) Si c'est le cas il doit bénéficier de la correction du patch, et donc ce serait plutôt se placer sous ce bloc et faire ``` if not chart.legend_at_bottom: # adapt height height = max(... ```
smihai commented 2023-03-17 11:16:04 +01:00
Author
Owner
Copy Link

Mais le graphe de type pie peut également voir sa légende déborder en hauteur, non ? (peut-être que je me plante)

En effet, il restait ce cas.
Je l'ai pris en compte et rajouté un test.

Pas repris ta suggestion d'implémentation car chart.legend_at_bottom lève un AttributeError.

> Mais le graphe de type pie peut également voir sa légende déborder en hauteur, non ? (peut-être que je me plante) > En effet, il restait ce cas. Je l'ai pris en compte et rajouté un test. Pas repris ta suggestion d'implémentation car `chart.legend_at_bottom` lève un `AttributeError`.
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from 7490f2c1cb to dfc4092d29 2023-03-17 11:10:14 +01:00 Compare
vdeniaud approved these changes 2023-03-27 11:26:53 +02:00
vdeniaud left a comment
Owner
Copy Link

Go, penser juste à ajouter la référence du ticket dans les titres de tous les commits (pinaillage à ce propos, plutôt « move » que « rename » dans le premier).

Go, penser juste à ajouter la référence du ticket dans les titres de tous les commits (pinaillage à ce propos, plutôt « move » que « rename » dans le premier).
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from dfc4092d29 to f356a5bd24 2023-03-27 11:32:28 +02:00 Compare
smihai force-pushed wip/62282-dataviz-allow-charts-auto-height from f356a5bd24 to 5f63dbbd3d 2023-03-27 11:34:23 +02:00 Compare
smihai commented 2023-03-27 11:43:53 +02:00
Author
Owner
Copy Link

Go, penser juste à ajouter la référence du ticket dans les titres de tous les commits (pinaillage à ce propos, plutôt « move » que « rename » dans le premier).

Remarques prises en compte.

> Go, penser juste à ajouter la référence du ticket dans les titres de tous les commits (pinaillage à ce propos, plutôt « move » que « rename » dans le premier). Remarques prises en compte.
smihai merged commit 5f63dbbd3d into main 2023-03-27 11:43:58 +02:00
smihai deleted branch wip/62282-dataviz-allow-charts-auto-height 2023-03-27 11:43:58 +02:00
Sign in to join this conversation.
Reviewers
No reviewers
vdeniaud
Labels
Clear labels
No items
No Label
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
3 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: entrouvert/combo#54
No description provided.
Delete Branch "wip/62282-dataviz-allow-charts-auto-height"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?