varvet / godmin

Admin framework for Rails 5+
http://godmin-sandbox.herokuapp.com
MIT License
486 stars 47 forks source link

Changes table row rendering into collection #200

Closed adimasuhid closed 7 years ago

adimasuhid commented 8 years ago

Moved rendering into collection. Apparently, the slow part in partial rendering is search for partials. Placing them in a collection (without nested partials) makes them smooth.

Original Speed (per 100 records): 1974 ms New Speed (per 100 records): 670 ms

Let me know what you think.

ref #177

adimasuhid commented 8 years ago

Though, realised this removes the partial override for columns/actions and instead moved it to override per row. Interested in your thoughts.

jensljungblad commented 8 years ago

Thanks for taking a look at this. I'm guessing its the removal of columns/actions that make it faster, and not extracting each row to its own partial? We've thought about this, but this is unfortunately backwards incompatible. Any major changes like this would have to go in a 2.0 release.

I still haven't looked into alternative approaches such as maybe customizing some parts of the GUI with presenters instead of partial overrides either, as discussed in the linked issue.

adimasuhid commented 8 years ago

Yes the removal of columns/actions make it faster. What if we preload the partial before the iteration of the rows, something like:

# app/views/godmin/resource/_table.html.erb
<% row_partial = lookup_context.find_template("#{controller_path}/columns/actions", lookup_context.prefixes, true, [:resource]) %>
<% @resources.each do |resource| %>
...
<%= row_partial.render(self, resource: resource) %>

Doesn't look as good due to the intermediary variable, but essentially preloads. Maybe a helper can help "hide" it. Otherwise, looks like presenters could be a good solution.

jensljungblad commented 8 years ago

Yeah we do cache it, https://github.com/varvet/godmin/pull/184 was merged a while back to master but I only released a new version just yesterday, so if you haven't checked it out yet please do!

jensljungblad commented 7 years ago

Closing for now