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

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

Merged
merged 3 commits into from
May 2, 2019

Conversation

sbull
Copy link
Contributor

@sbull sbull commented Apr 29, 2018

…mns and frozen row headers instead of paginated sets

Addresses Issue #2272.

@sbull
Copy link
Contributor Author

sbull commented May 2, 2018

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.

Copy link
Member

@mshibuya mshibuya left a comment

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.

app/views/rails_admin/main/index.html.haml Outdated Show resolved Hide resolved
lib/rails_admin/config.rb Show resolved Hide resolved
app/views/rails_admin/main/index.html.haml Show resolved Hide resolved
@sbull
Copy link
Contributor Author

sbull commented May 14, 2018

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

sbull commented May 20, 2018

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

sbull commented Dec 5, 2018

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

mshibuya commented May 2, 2019

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

@twikah
Copy link

twikah commented May 2, 2019

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