[openstreetmap/openstreetmap-website] Add pagination to Issues page (PR #5057)

[Tom Hughes]

@tomhughes requested changes on this pull request.

I think it might be better to split this into two commits - one that switches issues to use the common pagination helpers and a second to introduce turbo support.

> @@ -0,0 +1,40 @@
+<turbo-frame id="pagination" data-turbo-action="advance">
+  <% if @issues.length == 0 %>
+    <p><%= t(@user_not_found ? ".user_not_found" : ".issues_not_found") %></p>
+  <% else %>

I think this first branch should be outside the pagination frame because it's not actually paginated - only the else clause should be inside the frame as that is the part that can be navigated to different pages.

> @@ -0,0 +1,40 @@
+<turbo-frame id="pagination" data-turbo-action="advance">

This shouldn't have `data-turbo-action` but should have `target="_top"` and the advance action (and `pagination` as a target) should be on the next/previous buttons (which #4646 takes care of) so that other links inside the pagination section will break out to a full page load.

> @@ -39,6 +44,10 @@ def index
       last_updated_by = params[:last_updated_by].to_s == "nil" ? nil : params[:last_updated_by].to_i
       @issues = @issues.where(:updated_by => last_updated_by)
     end
+
+    @issues, @newer_issues_id, @older_issues_id = get_page_items(@issues, :limit => @params[:limit])
+
+    render :partial => "page" if turbo_frame_request_id == "pagination" || turbo_frame_request_id == "search"

I think the second part of this condition is wrong (in fact it's not clear to be the `search` frame is needed at all) and instead the search form should have a `data-turbo-frame="pagination"` attribute to target loading of the pagination frame.

> @@ -4,72 +4,42 @@
 
 <p><%= t ".search_guidance" %></p>
 
-<%= form_tag(issues_path, :method => :get) do %>
-  <div class="row gx-1">
-    <div class="mb-3 col-md-auto">
-      <%= select_tag :status,
-                     options_for_select(Issue.aasm.states.map(&:name).map { |state| [t(".states.#{state}"), state] }, params[:status]),
-                     :include_blank => t(".select_status"),
-                     :data => { :behavior => "category_dropdown" },
-                     :class => "form-select" %>
+<turbo-frame id="search">

Do we actually need this frame? When does it get updated?

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/5057#pullrequestreview-2244353971
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/5057/review/2244353971 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240818/0fd37314/attachment-0001.htm>