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

Fixes turbo page navigations by making all pages change browser history #1113

Closed
wants to merge 2 commits into from

Conversation

jasonfb
Copy link

@jasonfb jasonfb commented Oct 28, 2023

I think this is how Turbo is supposed to work and makes it so that the query parameters for each page are bookmarkable

@jasonfb
Copy link
Author

jasonfb commented Oct 28, 2023

this does not work as expected. there appears to be an entire subsystem in this code that downloads theme templates from this repo:
amatsuda/kaminari_themes

You can see this in the views_generator.rb file here:

https://github.com/kaminari/kaminari/blob/master/kaminari-core/lib/generators/kaminari/views_generator.rb#L122

This makes this gem dependent on the other gem, in a way that strikes me as oddly obfuscated.

I see the chicken & egg problem. I would suggest a unique approach to re-thinking this:

• All users install the kaminari gem by default.
• Any user who wants to have a custom theme also temporarily installs the kaminari_themes gem too.
• The generator command is moved into the amatsuda/kaminari_themes gem itself, because it is dependent on that gem anyway.
• You run the generator command and it copies the files from the gem code, instead of downloading them from Github. This fixes the issue of the current construction of having it always default to pull from the master branch of that repo. (That pattern seems to work OK for a solo project but it discourages other developers from — for example, building a new theme.)

Once you've copied the view files (like today— just coping from the gem code instead of downloading across GH API), then you eject the Gem you don't need— specifically, the amatsuda/kaminari_themes one, which is basically just a set of template files that don't need to live in your app codebase and get bundled with every deploy.

I think ejectable gems are underrated (very few people do this), but when you just want to copy code into the app, it seems appropriate.

@yuki24
Copy link
Member

yuki24 commented Feb 5, 2024

I don't think the default Turbo action should be 'advance' - that seems very application or context-specific and it totally depends on your use case. Could you explain why everyone, even the ones who may not use Hotwire/Turbo, is forced to use this?

@jasonfb
Copy link
Author

jasonfb commented Feb 5, 2024

@yuki24 - you are absolutely right that people who aren't using Turbo shouldn't need or use this at all.
this PR is quick-fix solution (hack), not a fully thought-out solution to solve the non-Turbo and the Turbo need.

Have you tried Kaminari in a Turbo app? The problem is very clear and specific: the pagination works, but when you advance page -to- page, the URL does not update. That means that if you reload the page or do another action, you go back to Page 1 which is counter-intuitive.

With 'data-turbo-action': 'advance`, every page navigation goes from page to page the way you'd expected a normal paginated interface to work-- when I navigate to page 2 or 3, I should stay on page 2 or 3 if reloading the window.

Because Kaminari actually copies these template files from the cloud (NOT from this repo)-- which I find odd honestly -- makes it so that this PR doesn't really do anything until it is published to the cloud (I tried forking but that didn't work because the code to copy the templates comes from the cloud not the local copy of the gem)

for background, please see this Hot Glue release, which is when/why I did this PR:

https://github.com/hot-glue-for-rails/hot-glue/releases/tag/v0.5.25

You'll notice that in my release notes I offer two different ways for the user to add the 'data-turbo-action': 'advanced' but neither of them involves this PR.

A quick fix is simply this:

rails g kaminari:views bootstrap4
sed -i '' -e "s/class: 'page-link',/class: 'page-link', 'data-turbo-action': 'advance'/g" app/views/kaminari/_first_page.html.erb app/views/kaminari/_gap.html.erb app/views/kaminari/_last_page.html.erb app/views/kaminari/_next_page.html.erb app/views/kaminari/_page.html.erb app/views/kaminari/_prev_page.html.erb

@yuki24
Copy link
Member

yuki24 commented Feb 5, 2024

I am using Turbo Drive and Kaminari on https://toprubycompanies.info/consultancies and it's totally working fine without any data-turbo attributes.

@yuki24
Copy link
Member

yuki24 commented Feb 8, 2024

I'm closing this issue as this may not seem universal enough. There is also a more generic PR #1095 and I would like to continue the discussion there.

@yuki24 yuki24 closed this Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants