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

Compatibility with rails 5.1 #135

Open
vinc opened this issue Dec 6, 2017 · 13 comments
Open

Compatibility with rails 5.1 #135

vinc opened this issue Dec 6, 2017 · 13 comments

Comments

@vinc
Copy link

vinc commented Dec 6, 2017

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 #40 (comment) and maybe #85

@esb
Copy link

esb commented Jan 9, 2018

This gem seems to be abandonware now.

@ljachymczyk
Copy link
Member

ljachymczyk commented Jan 9, 2018

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
Copy link

esb commented Jan 9, 2018

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
Copy link
Author

vinc commented Jan 9, 2018

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
Copy link

mizinsky commented Jan 12, 2018

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
Copy link
Author

vinc commented Jan 12, 2018

@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
Copy link

@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 (smart_listing.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
Copy link
Author

vinc commented Jan 15, 2018

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
Copy link

esb commented Jan 15, 2018

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
Copy link
Author

vinc commented Jan 15, 2018

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
Copy link
Member

@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.

@ljachymczyk ljachymczyk reopened this Jan 15, 2018
@jtoly
Copy link

jtoly commented Jan 16, 2018

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
Copy link
Member

Readme just got updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants