Sology / smart_listing

Ruby on Rails data listing gem with built-in sorting, filtering and in-place editing.
http://showcase.sology.eu/smart_listing
MIT License
478 stars 138 forks source link

Compatibility with rails 5.1 #135

Open vinc opened 6 years ago

vinc commented 6 years ago

Hey there, first of all thanks for this gem that allows us have lightweight controllers without logic dealing with sort, filter and pagination!

I'm using rails 5.1.4 on a project and I can't figure out the proper setup.

If I install jquery with yarn (for bootstrap) and keep using //= require rails-ujs the sort and pagination work but I get a TypeError: element.attr is not a function when I click on a table header to sort (but it works), and rails stuff like confirm dialog stop working with the same error.

If I install jquery-ujs with yarn and replace //= require rails-ujs with //= require jquery-ujs I don't have the broken confirm dialog anymore in other part of my app but nothing related to smart_listing work. When I click to sort nothing happen (no errors) and when I click on the pagination the opacity of table is decreased but nothing happen (no errors).

If I install the gem jquery-rails and replace //= require rails-ujs with //= require jquery_ujs it's the same thing.

My understanding was that I needed to replace //= require rails-ujs with //= require jquery-ujs (or //= require jquery_ujs depending on where I get it from) to get smart_listing happy but that's not happening with rails 5.1.4.

See https://github.com/Sology/smart_listing/issues/40#issuecomment-335041373 and maybe https://github.com/Sology/smart_listing/issues/85

esb commented 6 years ago

This gem seems to be abandonware now.

ljachymczyk commented 6 years ago

No it isn't. Just don't have time right now to work on it. But hey @esb happy to accept pull requests though!

esb commented 6 years ago

I've just been battling a similar problem with Rails 5.1 and rails_ujs, so I'm hoping you can find the time to have a look at it as it would be nice to have the gem working with the current version of Rails.

I've been trying to get the Control Form working, and it has been crashing in the rails_ujs handleRemote function. I switched back to the jquery_ujs version and it's working, so I don't know what the problem is.

I think you'd have some grateful users, if you could find time to have a look at this.

vinc commented 6 years ago

I haven't found a fix for this issue so I disabled remote loading:

$(document).on("turbolinks:load", function() {
  // Disable ajax remote loading of smart listing links
  // See https://github.com/sology/smart_listing/issues/135
  $(".smart-listing a").on("click", function(e) {
    e.preventDefault();
    Turbolinks.visit(this.href);
  });
});
mizinsky commented 6 years ago

smart_listing requires a jquery-rails gem to work correctly. That is why you have to check if your application.js file contains:

//= require jquery 
//= require jquery_ujs

@esb @vinc does jquery-rails gem and adding these lines to application.js file help?

vinc commented 6 years ago

@mizinsky this doesn't work for me and that's what prompted me to create this issue. See above for a detailed explanation of the problem.

mizinsky commented 6 years ago

@vinc smart_listing requires jquery-rails gem for proper functioning. The only way I received this error

TypeError: element.attr is not a function

was when I haven't properly installed jquery-rails gem and didn't include:

//= require jquery 
//= require jquery_ujs

in my application.js file, before //= require smart_listing

.attr() is a jQuery function, and your error may appear due to this function (_smartlisting.coffee.erb file):

$.rails.href = (element) ->
  element.attr("href") || element.data("<%= SmartListing.config.data_attributes(:href) %>") || window.location.pathname

rails-ujs don't provide jQuery, and that is why we need jquery-rails gem working properly. In the future, smart_listing plans to get rid of jQuery code, and any jQuery-related dependencies.

vinc commented 6 years ago

Yes, the first part of my first message describe the error when I don't use jquery-rails, but as explained in the reminding of the message, I tried using jquery-rails and while I don't get the error anymore, it doesn't fix the problem I'm reporting here:

If I install jquery-ujs with yarn and replace //= require rails-ujs with //= require jquery-ujs I don't have the broken confirm dialog anymore in other part of my app but nothing related to smart_listing work. When I click to sort nothing happen (no errors) and when I click on the pagination the opacity of table is decreased but nothing happen (no errors).

If I install the gem jquery-rails and replace //= require rails-ujs with //= require jquery_ujs it's the same thing.

esb commented 6 years ago

Closing this issue doesn't solve the problem.

There's a big misunderstanding here. jquery_ujs does not provide jQuery. It is merely the set of tools for UJS that rails needs that was written using jQuery. In Rails 5, this was re-written as rails_ujs and it doesn't have a dependency on jQuery.

That being said, if you need jQuery under Rails 5, just include it via yarn and use rails_ujs for the UJS support.

The issue here is that something breaks with smart_listing when using the rails_ujs code, and that it the problem. It means the code is not compatible with Rails 5.

vinc commented 6 years ago

With rails_ujs the code definitely breaks, but even when I replace it with jquery_ujs it's still not working for me on rails 5.1. Do you experience the same thing @esb?

Maybe I should produce a small test case to reproduce the problem.

ljachymczyk commented 6 years ago

@esb SmartListing in its current state requires jQuery to operate. This means you need to use jquery_ujs instead of rails_ujs (and yes, we rely on some code provided by jquery_ujs too). We're going to add this requirement to the Readme soon. @vinc in order to debug your problem, could you please do following:

  1. Prepare the scenario when you where you use jquery_ujs and don't see the 'TypeError'
  2. Open developer console in your browser and observe network requests.
  3. Try to sort by some table header
  4. Find the corresponding sort XHR request that you just triggered by clicking on sort header
  5. Copy it's response body (javascript code)
  6. Paste the code into JS console and execute it - you should see some errors; please paste them here.
jtoly commented 6 years ago

For what it's worth, and know that I've only been looking into getting pagination to work as intended, I altered smart_listing.coffee.erb:4 to check only that element.href exists rather than that element.attr("href") exists:

$.rails.href = (element) ->
  element.href || element.data("<%= SmartListing.config.data_attributes(:href) %>") || window.location.pathname

And since I struggled to get $.rails.href to be defined properly, my application.js requires in order:

//
//= require turbolinks
//= require jquery
//= require rails-ujs
//= require smart_listing
//= require_tree .

I have a lot more to look into (my search for gems supporting edit in place is what led me to SmartListing) so if I encounter and resolve other issues, I'll return here to see if the conversation is continuing! Thank you, @mizinsky for the gem, and everyone else for the discussion.

ljachymczyk commented 6 years ago

Readme just got updated.