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

FIX: 1631 #1633

Draft
wants to merge 6 commits into
base: main
Choose a base branch
from
Draft

FIX: 1631 #1633

wants to merge 6 commits into from

Conversation

MrTob
Copy link
Contributor

@MrTob MrTob commented Jan 19, 2025

Fix rows per page selection in account entries (#1631 )

The "rows per page" selector in account entries didn't work correctly because the form was submitting to the wrong endpoint. This was happening because it used custom_pagy_url_for which directed the request through the parent resource's show action instead of the entries endpoint.

Changes

  • Modified the form in _pagination.html.erb to submit directly to account_entries_path with the correct account_id
  • Added turbo_frame targeting to ensure proper frame updates
  • Added explicit parameter preservation for tab and search context
  • Removed custom_pagy_url_for usage from the form (kept for pagination links)

The form now correctly submits to the entries endpoint while preserving all necessary context parameters, allowing proper rows per page updates within the Turbo Frame.

Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

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

@MrTob this is definitely the cause of the bug, but I think we should be careful adding to much responsibility to this _pagination.html.erb partial. This partial is meant to be agnostic of what view it is rendered in, so I don't think we should be adding checks such as defined?(@account) ? account_entries_path(...) : ...

For the moment, I think a clearer option would be to duplicate this partial into an entry-specific one, such as views/account/entries/_pagination.html.erb

That way, we can be highly specific with our intended routes and remove a lot of the conditional logic.

Yes, this will result in a bit of code duplication, but I think the clarity tradeoff is worth it at this stage.

@zachgoll
Copy link
Collaborator

@MrTob be sure to request a review when this is ready again and I can take a final look!

@MrTob MrTob marked this pull request as draft January 21, 2025 14:59
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.

2 participants