patricklindsay / wice_grid

A Rails grid plugin to create grids with sorting, pagination, and (automatically generated) filters
http://wicegrid.herokuapp.com/
MIT License
33 stars 29 forks source link

Init wice grid on turbolinks page load event #5

Closed the-undefined closed 6 years ago

the-undefined commented 6 years ago

Columns with auto_reload: true on them are not working on initial page load. This looks to be because:

Turbolinks overrides the normal page loading process

(http://guides.rubyonrails.org/working_with_javascript_in_rails.html#page-change-events)

So attach initWiceGrid callback to the turbolinks:load event instead of the turbolinks:render event.

The turbolinks:render event only fires when the page is updated. The turbolinks:load event is fired after each page change, which includes the initial page load.

From the turbolinks source:

translateEvent from: "turbolinks:render", to: "page:update"
translateEvent from: "turbolinks:load", to: "page:change"

(https://github.com/turbolinks/turbolinks/blob/c73e134731ad12b2ee987080f4c905aaacdebba1/src/turbolinks/compatibility.coffee#L16-L17)


P.S Thanks for updating wice to work with rails 5 @patricklindsay :)

patricklindsay commented 6 years ago

Thanks for the contribution! I'll take a look at it properly and get back to you.

the-undefined commented 6 years ago

Thanks, I wasn't sure if I was missing something around the page:load ready event.

patricklindsay commented 6 years ago

I can't see how page:load ready isn't handling these auto_reload columns.

Could you create a demo app & write steps to reproduce the issue?

From the looks of it this change could cause a double initialise. My knowledge on this codebase is (extremely) limited so not sure how much of a problem this would be.

the-undefined commented 6 years ago

@patricklindsay I've put together a demo application here: https://github.com/the-undefined/wice_grid_pr_5_demo and the reproduction steps are in the readme. In doing so I also carried on the investigation and found the cause.

In the demo app I created a branch without-turbolinks (obvs I removed turbolinks here) and the problem is still present. So the issue is with page:load ready not firing.

In that branch I am requiring jquery3 in the app/assets/application.js, so I decided to try an older version of jquery. In the jquery2 branch the auto_reload is working.

So there seems to be an incompatibility with the event page:load ready and jQuery3. I've run out of investigation time today, but I will continue looking into this, so this is my status report :)

the-undefined commented 6 years ago

So $(document).on('ready') has been deprecated in jQuery3, see https://jquery.com/upgrade-guide/3.0/#deprecated-document-ready-handlers-other-than-jquery-function, because this is now an async event which semantically breaks compatibility with previous versions.

I've updated this PR to use $(fn) variant instead, which works across all versions of jQuery, but I need to do more investigation into the side effects of the jQuery3 version being asyc.

I tried to find information on the page:load event that I have dropped but I couldn't find docs or references for this anywhere - let me know if you have information on using this alongside the ready event.

I have also reverted the turbolink event to use turbolinks:render.

I've tested these changes in the demo app against:

Next I will test against an application that is using images and date pickers.

the-undefined commented 6 years ago

Found where page:load is coming from: https://github.com/turbolinks/turbolinks/blob/c73e134731ad12b2ee987080f4c905aaacdebba1/src/turbolinks/compatibility.coffee#L26 it is dispatched from turbolinks.

patricklindsay commented 6 years ago

The reason why both page:load and ready were being listened to was for compatibility with Turbolinks & when users are loading their JS asynchronously.

The page:load would be fired by Turbolinks but it wouldn't be fired if JS was async - which is where ready came to the rescue.

Will need to make sure this still works when JS is async'd

the-undefined commented 6 years ago

Hey @patricklindsay, it looks like turbolinks and async is an unsolved problem atm: https://github.com/turbolinks/turbolinks/issues/281 at least for v5. It's quite the rabbit hole and async scripts are not something I have tried using before.

However, to experiment on the demo app I added this commit to all branches:

diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb
index 8a0fbc8..3326314 100644
--- a/app/views/layouts/application.html.erb
+++ b/app/views/layouts/application.html.erb
@@ -5,7 +5,7 @@
     <%= csrf_meta_tags %>

     <%= stylesheet_link_tag    'application', media: 'all', 'data-turbolinks-track': 'reload' %>
-    <%= javascript_include_tag 'application', 'data-turbolinks-track': 'reload' %>
+    <%= javascript_include_tag 'application', 'data-turbolinks-track': 'reload', async: true %>
   </head>

   <body>
diff --git a/config/environments/development.rb b/config/environments/development.rb
index 5187e22..c1be2c4 100644
--- a/config/environments/development.rb
+++ b/config/environments/development.rb
@@ -40,7 +40,7 @@ Rails.application.configure do
   # Debug mode disables concatenation and preprocessing of assets.
   # This option may cause significant delays in view rendering with a large
   # number of complex assets.
-  config.assets.debug = true
+  config.assets.debug = false

   # Suppress logger output for asset requests.
   config.assets.quiet = true

Turning off asset debugging prevented all of the js scripts being loaded individually and out of sync - which crashed more than just wice grid.

Wice Grid and auto_reload still worked with the above diff for all of the combinations for jQuery and turbolinks being enabled / disabled that I mentioned in the previous comments. So by using $(fn) it looks like we are still covered.

Are there any other tests that you would like me to run on this change? JS isn't something I am knowledgable about so I'll take whatever direction you have in mind.

miguelac commented 6 years ago

Was having issues with filter panel show/hide button and csv download button not working after a page reload. Did a couple of tests using this PR and it is working now. 👏