Skip to content

Add config.horizontal_scroll_list to enable horizontal scrolling colu…#3017

Merged
mshibuya merged 3 commits into
railsadminteam:masterfrom
sbull:horizontal-scroll-list
May 2, 2019
Merged

Add config.horizontal_scroll_list to enable horizontal scrolling colu…#3017
mshibuya merged 3 commits into
railsadminteam:masterfrom
sbull:horizontal-scroll-list

Conversation

@sbull

@sbull sbull commented Apr 29, 2018

Copy link
Copy Markdown
Contributor

…mns and frozen row headers instead of paginated sets

Addresses Issue #2272.

@sbull sbull force-pushed the horizontal-scroll-list branch 4 times, most recently from bcfb492 to a03b4b4 Compare May 1, 2018 22:51
@sbull

sbull commented May 2, 2018

Copy link
Copy Markdown
Contributor Author

Hi @mshibuya, I've created this PR due to interest from Issue #2272 and I've updated the wiki https://github.com/sferik/rails_admin/wiki/Horizontally-scrolling-table-with-frozen-columns-in-list-view to describe it. Hopefully this can make it into a release.

The current Travis failures seem unrelated to this PR - "unable to load puma" on other test cases.

@mshibuya mshibuya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, but there's a few things to consider.

Comment thread app/views/rails_admin/main/index.html.haml Outdated
Comment thread lib/rails_admin/config.rb
Comment thread app/views/rails_admin/main/index.html.haml
@sbull

sbull commented May 14, 2018

Copy link
Copy Markdown
Contributor Author

Thanks for your feedback!

  1. I had wanted to make this as low-impact as possible, so that the JS & CSS wouldn't even be loaded in the default configuration. If these are in separate files, I don't think it's possible to conditionally load them (assuming the right thing would be to make them part of the asset precompile pipeline)? But I can submit a change to do it the "right" way.

  2. I expect that the most-used config will be global options, but I will look into making it work per-model as well.

  3. I'll look into moving some of the calculation logic into the config class or helpers.

I hope to put some time into this in a few days. Thanks again!

@sbull sbull force-pushed the horizontal-scroll-list branch from a03b4b4 to d511913 Compare May 19, 2018 20:27
@sbull

sbull commented May 20, 2018

Copy link
Copy Markdown
Contributor Author

Hi @mshibuya, I've refactored this code to address your requests

  1. Move JS & CSS into asset files.
  2. Enable configuration at model level.
  3. Move config logic out of view.

I've reworked the tests to account for these changes, and created new tests to check the model-level configuration. I've also rebased this PR. I can squash these commits if you'd like but I thought I'd leave them separate for now for you to review the changes from the original PR.

Let me know if other changes are needed. I don't really know how to fix the codeclimate "cognitive complexity" issue - perhaps split it up into multiple methods, but that seems like it could result in a fair amount of duplicate code.

Thanks!

@sbull

sbull commented Dec 5, 2018

Copy link
Copy Markdown
Contributor Author

Hi @mshibuya , I was wondering if you could take a look at this, as it's been open for a while. Thanks!

@mshibuya mshibuya added this to the 2.0.0 milestone May 2, 2019
@mshibuya mshibuya merged commit c549560 into railsadminteam:master May 2, 2019
@mshibuya

mshibuya commented May 2, 2019

Copy link
Copy Markdown
Member

Thanks you for amazing work!
I'm planning to change the name of this feature to sidescroll.

@twikah

twikah commented May 2, 2019

Copy link
Copy Markdown

Hi @mshibuya, thanks for getting this merged! Are you planning on making a release soon? It would be great!

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

Successfully merging this pull request may close these issues.

3 participants