search

airblade / chartjs-ror

[Not supported - see #53] Simplifies using Chart.js in Rails
MIT License
184 stars 57 forks source link

Fix issues with Turbolinks #20

Closed ledermann closed 9 years ago

ledermann commented 9 years ago

There are two issues using the chart helper in conjunction with Turbolinks:

  1. On using browser's back button, Turbolinks restores the page from a cache and triggers a page:restore event instead of page:load. Because there is no listener for this event, the charts on this pages are missing. Maybe this could be fixed by adding a listener for page:restore - but wait and look at the next issue.
  2. Adding event listeners seems not right to me (disclaimer: I'm not a JS expert): If the user changes to a page without charts, the added listeners still exists and are triggered again - but fail because the chart does not exist in the DOM anymore. In the browsers JS console we see JS error on every page change.

This PR fixes both issues by executing initChart() without event listener (like proposed by @glundgren in #5). Because #5 is some kind of stale, I open this one. What do you think?

The right way of initializing a chart is asked in nnnick/Chart.js#929, but there is still no anwer.

airblade commented 9 years ago

I tried your change but it didn't work for me on a page where the chart is part of the initial page, not loaded later via AJAX. My third-party JavaScripts are included at the bottom of the page so when the chart is written out in the middle of the page, and the initChart() executes immediately, the Chart variable hasn't yet been defined.

Maybe the we could listen for the page:load and page:restore events, but only once. I.e. the last line of the initChart() function would unbind those listeners. I'm not sure, my JS isn't very good...

airblade commented 9 years ago

Please can you try this?

diff --git a/lib/chartjs/chart_helpers.rb b/lib/chartjs/chart_helpers.rb
index eb389f1..5419c00 100644
--- a/lib/chartjs/chart_helpers.rb
+++ b/lib/chartjs/chart_helpers.rb
@@ -55,15 +55,20 @@ module Chartjs

             #{legend if generate_legend}
           };
-          /* W3C standard */
-          if (window.addEventListener) {
-            window.addEventListener("load", initChart, false);
-            document.addEventListener("page:load", initChart, false);
+          if (typeof Chart !== "undefined" && Chart !== null) {
+            initChart();
           }
-          /* IE */
-          else if (window.attachEvent) {
-            window.attachEvent("onload", initChart);
-            document.attachEvent("page:load", initChart);
+          else {
+            /* W3C standard */
+            if (window.addEventListener) {
+              window.addEventListener("load", initChart, false);
+              document.addEventListener("page:load", initChart, false);
+            }
+            /* IE */
+            else if (window.attachEvent) {
+              window.attachEvent("onload", initChart);
+              document.attachEvent("page:load", initChart);
+            }
           }
         })();
         END

If Chart is already defined it executes initChart() immediately. This takes care of the AJAX case.

If Chart isn't defined, it waits for the load event before calling initChart(). This takes care of normal page loads.

ledermann commented 9 years ago

Ok, I've only tested my change in my app where Turbolinks is used, so the JavaScripts are loaded in the head, not in the bottom. You are right, loading the JS later in the <body> will break my change.

I have tried your patch, it works fine in my app: It always executes the if-part, the else-part is never used.

I'm not sure if we need the page:load listener anymore. It may be required if Turbolinks is used and the JS is loaded later in the <body>, which should be avoided.

airblade commented 9 years ago

Good to hear the patch works for you.

I need the page:load listener in my app which doesn't yet use Turbolinks and loads JS later on in the body. Without it, initChart() is never called. Would you agree?

ledermann commented 9 years ago

Hm, so far as I know, page:load is an event triggered by Turbolinks. Without Turbolinks you need a listener for onload only (which of course should not be removed).

airblade commented 9 years ago

Aha, you're right. I had forgotten that page:load came from Turbolinks.

I'll release a new version of the gem with this change.

ledermann commented 9 years ago

Great, thanks!

airblade commented 9 years ago

It's in v2.1.3. Thanks for all your help!

smikkelsen commented 9 years ago

I can confirm this fixed my problem. (https://github.com/airblade/chartjs-ror/issues/18) Thanks so much guys!