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

Enlève jQuery de js/charts.js et met à jour Chart.js #6114

Merged
merged 4 commits into from
Sep 23, 2021

Conversation

philippemilink
Copy link
Member

@philippemilink philippemilink commented May 4, 2021

Fix #6113

On va attendre que la PR sur Matomo soit mergée. C'est fait.

  • Supprime la dépendance à jQuery dans le script JS qui gère les graphes des statistiques
  • Met à jour ChartJS à la version 3
  • Ajoute une légende aux axes des ordonnées
  • Corrige un bug d'affichage lorsqu'on passe sur un nouvel onglet pour la première fois (le graphe n'est pas encore généré, et l'affichage soudain du graphe produit un effet bizarre). Pour reproduire: afficher la page des statistiques, changer d'onglet, changer le style du graphique (dans le menu de gauche: ligne ou histogramme), changer ensuite d'onglet: l'affichage du graphique de l'onglet sélectionné n'est pas fluide: il apparaît brutalement. Le problème se produit aussi en changeant la taille de la fenêtre.

Je laisse tomber le dernier point: le problème me semble compliqué à résoudre, n'est pas vraiment gênant et n'est pas spécifique à cette PR/issue. Le problème provient du fait que Chart.js ne peut pas générer les graphes qui ne sont pas affichés (ici les onglets inactifs), parce qu'il ne connaît pas la taille que va avoir le graphique. J'ai essayé de jouer avec chart.resize(width, height), mais sans succès. Une autre solution serait sans doute: à chaque changement de style de graphe ou de changement de taille (fenêtre redimensionnée), on affiche tous les onglets, génère les graphes, puis affiche seulement l'onglet courant. Ça me semble être un peu lourd comme solution...

Contrôle qualité

Je n'ai pas vraiment pu tester le contenu des graphes (en local, ce n'est pas évident), il faudra tester en bêta.

Tout ce qui touche au JavaScript:

  • la navigation par onglets
  • le changement du style de graphe (ligne ou histogramme) et l'affichage choisi est retenu (persiste au rechargement de la page)
  • les graphiques s'affichent bien
  • regarder également sur les autres pages s'il n'y a pas d'erreurs JavaScript dans la console

Les données affichées dans les graphes:

  • sont bien affichées
  • notamment le cas où il y a plusieurs courbes par graphique (cas que je ne peux pas tester moi-même, je n'ai pas de big-tuto)
  • les dates de l'axe des abscisses et des tooltips sont bien formatées

@coveralls
Copy link

coveralls commented May 5, 2021

Coverage Status

Coverage remained the same at 86.621% when pulling acde43a on philippemilink:chartjs into 9cbde43 on zestedesavoir:dev.

@philippemilink philippemilink marked this pull request as ready for review May 8, 2021 11:28
@artragis
Copy link
Member

tu peux résoudre les conflits que je vois si je peux tester ça dans la semaine?

@philippemilink
Copy link
Member Author

Je viens de me mettre à jour par rapport à la branche dev, par contre je viens de trouver un bug que je n'arrive pas à résoudre pour l'instant:

  • charger une page de statistiques
  • passer d'un graphe à l'autre avec les onglets
  • changer le style du graphique (courbe à histogramme, ou l'inverse)
  • un graphique a alors une mauvaise légende sur l'axe des ordonnées

@Situphen
Copy link
Member

Je viens de me mettre à jour par rapport à la branche dev, par contre je viens de trouver un bug que je n'arrive pas à résoudre pour l'instant:

* charger une page de statistiques

* passer d'un graphe à l'autre avec les onglets

* changer le style du graphique (courbe à histogramme, ou l'inverse)

* un graphique a alors une mauvaise légende sur l'axe des ordonnées

J'ai pu investiguer et résoudre le soucis. Étant donné qu'on définit basicOptions en dehors de la fonction setupChart puis qu'on l'utilise lors de la création de config, j'ai l'impression que config.options.scales.y.title ne surcharge pas toujours la variable config. La solution simple est de définir le contenu de basicOptions directement à la création de config, mais c'est sympa d'avoir la config tout en haut du fichier. La solution que je propose c'est d'utiliser la décomposition :

diff --git a/assets/js/charts.js b/assets/js/charts.js
index 1ee31365f..c9135d1b3 100644
--- a/assets/js/charts.js
+++ b/assets/js/charts.js
@@ -120,11 +120,17 @@
         labels: times,
         datasets: data
       },
-      options: basicOptions
-    }
-    config.options.scales.y.title = {
-      display: true,
-      text: chartEl.getAttribute('data-y-label')
+      options: {
+        ...basicOptions,
+        scales: {
+          y: {
+            title: {
+              display: true,
+              text: chartEl.getAttribute('data-y-label')
+            }
+          }
+        }
+      }
     }
     charts.push(new window.Chart(chartEl, config))
   }

@philippemilink
Copy link
Member Author

J'ai committé ton patch, à l'aveugle, je te laisse tester sur la bêta.

Copy link
Member

@Situphen Situphen left a comment

Choose a reason for hiding this comment

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

QA OK pour moi !

@Situphen Situphen merged commit 01ee0fa into zestedesavoir:dev Sep 23, 2021
@philippemilink philippemilink deleted the chartjs branch September 23, 2021 20:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Mettre à jour Chart.js
4 participants