Display changeset element pagination vertically

app/helpers/browse_helper.rb

@@ -70,37 +70,39 @@ def link_follow(object)
     "nofollow" if object.tags.empty?
   end
 
-  def type_and_paginated_count(type, pages)
+  def type_and_paginated_count(type, pages, selected_page = pages.current_page)
     if pages.page_count == 1
       t ".#{type.pluralize}",
         :count => pages.item_count
     else
       t ".#{type.pluralize}_paginated",
-        :x => pages.current_page.first_item,
-        :y => pages.current_page.last_item,
+        :x => selected_page.first_item,
+        :y => selected_page.last_item,
         :count => pages.item_count
     end
   end
 
   def sidebar_classic_pagination(pages, page_param)
-    max_width_for_default_padding = 35
-
-    width = 0
-    pagination_items(pages, {}).each do |body|
-      width += 2 # padding width
-      width += body.length
-    end
-    link_classes = ["page-link", { "px-1" => width > max_width_for_default_padding }]
-
-    tag.ul :class => "pagination pagination-sm mb-1 ms-auto" do
-      pagination_items(pages, {}).each do |body, n|
-        linked = !(n.is_a? String)
+    items = pagination_items(pages, {})
+    common_link_classes = %w[page-link ms-0]
+    above_active = true
+    below_active = false
+
+    tag.ul :class => "pagination pagination-sm flex-column text-center" do
+      items.each_with_index do |(body, page_or_class), i|
+        linked = !(page_or_class.is_a? String)
+        above_active = false if !linked && page_or_class == "active"
+        link_classes = common_link_classes + [{ "rounded-top rounded-bottom-0" => i.zero?,
+                                                "rounded-top-0 rounded-bottom" => i == items.count - 1,
+                                                "border-bottom-0" => above_active,
+                                                "border-top-0" => below_active }]
         link = if linked
-                 link_to body, url_for(page_param => n), :class => link_classes
+                 link_to body, url_for(page_param => page_or_class.number), :class => link_classes, :title => yield(page_or_class)
                else
                  tag.span body, :class => link_classes
                end
-        concat tag.li link, :class => ["page-item", { n => !linked }]
+        concat tag.li link, :class => ["page-item", { page_or_class => !linked }]
+        below_active = true if !linked && page_or_class == "active"
       end
     end
   end

app/views/changesets/_elements.html.erb

@@ -0,0 +1,17 @@
+<div class="d-flex gap-2">
+  <div class="flex-grow-1">
+    <h4 class="fs-5"><%= type_and_paginated_count(type, pages) %></h4>
+    <ul class="list-unstyled">
+      <% elements.each do |element| %>
+        <%= element_list_item type, element do
+              t "printable_name.current_and_old_links_html",
+                :current_link => link_to(printable_element_name(element), :controller => :browse, :action => type, :id => element.id[0]),
+                :old_link => link_to(printable_element_version(element), :controller => "old_#{type.pluralize}", :action => :show, :id => element.id[0], :version => element.version)
+            end %>
+      <% end %>
+    </ul>
+  </div>
+  <% if pages.page_count > 1 %>
+    <%= sidebar_classic_pagination(pages, "#{type}_page") { |page| type_and_paginated_count(type, pages, page) } %>
+  <% end %>
+</div>

app/views/changesets/show.html.erb

@@ -77,42 +77,21 @@
   <% end %>
 
   <% unless @ways.empty? %>
-    <%= render :partial => "paging_nav", :locals => { :type => "way", :pages => @way_pages } %>
-    <ul class="list-unstyled">
-      <% @ways.each do |way| %>
-        <%= element_list_item "way", way do
-              t "printable_name.current_and_old_links_html",
-                :current_link => link_to(printable_element_name(way), way_path(way.way_id)),
-                :old_link => link_to(printable_element_version(way), old_way_path(way.way_id, way.version))
-            end %>
-      <% end %>
-    </ul>
+    <section id="changeset_ways">
+      <%= render :partial => "elements", :locals => { :type => "way", :elements => @ways, :pages => @way_pages } %>
+    </section>
   <% end %>
 
   <% unless @relations.empty? %>
-    <%= render :partial => "paging_nav", :locals => { :type => "relation", :pages => @relation_pages } %>
-    <ul class="list-unstyled">
-      <% @relations.each do |relation| %>
-        <%= element_list_item "relation", relation do
-              t "printable_name.current_and_old_links_html",
-                :current_link => link_to(printable_element_name(relation), relation_path(relation.relation_id)),
-                :old_link => link_to(printable_element_version(relation), old_relation_path(relation.relation_id, relation.version))
-            end %>
-      <% end %>
-    </ul>
+    <section id="changeset_relations">
+      <%= render :partial => "elements", :locals => { :type => "relation", :elements => @relations, :pages => @relation_pages } %>
+    </section>
   <% end %>
 
   <% unless @nodes.empty? %>
-    <%= render :partial => "paging_nav", :locals => { :type => "node", :pages => @node_pages } %>
-    <ul class="list-unstyled">
-      <% @nodes.each do |node| %>
-        <%= element_list_item "node", node do
-              t "printable_name.current_and_old_links_html",
-                :current_link => link_to(printable_element_name(node), node_path(node.node_id), { :rel => link_follow(node) }),
-                :old_link => link_to(printable_element_version(node), old_node_path(node.node_id, node.version), { :rel => link_follow(node) })
-            end %>
-      <% end %>
-    </ul>
+    <section id="changeset_nodes">
+      <%= render :partial => "elements", :locals => { :type => "node", :elements => @nodes, :pages => @node_pages } %>
+    </section>
   <% end %>
 </div>
 

config/locales/en.yml

@@ -477,7 +477,7 @@ en:
       hidden_comment_by_html: "Hidden comment from %{user} %{time_ago}"
       changesetxml: "Changeset XML"
       osmchangexml: "osmChange XML"
-    paging_nav:
+    elements:
       nodes: "Nodes (%{count})"
       nodes_paginated: "Nodes (%{x}-%{y} of %{count})"
       ways: "Ways (%{count})"

lib/classic_pagination/pagination_helper.rb

@@ -145,21 +145,21 @@ def pagination_items(paginator, options)
         items = []
 
         if always_show_anchors && !(wp_first = window_pages[0]).first?
-          items.push [first.number.to_s, first.number]
+          items.push [first.number.to_s, first]
           items.push ["...", "disabled"] if wp_first.number - first.number > 1
         end
 
         window_pages.each do |page|
           if current_page == page && !link_to_current_page
             items.push [page.number.to_s, "active"]
           else
-            items.push [page.number.to_s, page.number]
+            items.push [page.number.to_s, page]
           end
         end
 
         if always_show_anchors && !(wp_last = window_pages[-1]).last?
           items.push ["...", "disabled"] if last.number - wp_last.number > 1
-          items.push [last.number.to_s, last.number]
+          items.push [last.number.to_s, last]
         end
 
         items

test/system/changeset_elements_test.rb

@@ -0,0 +1,41 @@
+require "application_system_test_case"
+
+class ChangesetElementsTest < ApplicationSystemTestCase
+  test "can navigate between element subpages without losing comment input" do
+    element_page_size = 20
+    changeset = create(:changeset, :closed)
+    ways = create_list(:way, element_page_size + 1, :with_history, :changeset => changeset)
+    way_paths = ways.map { |way| way_path(way) }
+    nodes = create_list(:node, element_page_size + 1, :with_history, :changeset => changeset)
+    node_paths = nodes.map { |node| node_path(node) }
+
+    sign_in_as(create(:user))
+    visit changeset_path(changeset)
+
+    within_sidebar do
+      assert_one_missing_link way_paths
+      assert_link "Ways (21-21 of 21)"
+
+      assert_one_missing_link node_paths
+      assert_link "Nodes (21-21 of 21)"
+    end
+  end
+
+  private
+
+  def assert_one_missing_link(hrefs)
+    missing_href = nil
+    hrefs.each do |href|
+      missing = true
+      assert_link :href => href, :minimum => 0, :maximum => 1 do
+        missing = false
+      end
+      if missing
+        assert_nil missing_href, "unexpected extra missing link '#{href}'"
+        missing_href = href
+      end
+    end
+    assert_not_nil missing_href, "expected one link missing but all are present"
+    missing_href
+  end
+end