[openstreetmap/openstreetmap-website] Traces: Migrate list to bootstrap (#3036)

[Andy Allan]

@gravitystorm requested changes on this pull request.

Overall I like parts of this PR, e.g. removing the `...` and making the link text clearer. But I'm not convinced by the list vs tables - not yet, but still a possibility!

> -        <a href="<%= url_for :controller => "traces", :action => "show", :id => trace.id, :display_name => trace.user.display_name %>"><img src="<%= url_for :controller => "traces", :action => "icon", :id => trace.id, :display_name => trace.user.display_name %>" border="0" alt="" /></a>
-      <% else %>
-        <span class="text-danger"><%= t ".pending" %></span>
-      <% end %>
+<li class="media mb-1 p-2 <%= cycle("", " bg-light") %>">
+  <% if Settings.status != "gpx_offline" %>
+    <% if trace.inserted %>
+      <a href="<%= url_for :controller => "traces", :action => "show", :id => trace.id, :display_name => trace.user.display_name %>">
+        <img src="<%= url_for :controller => "traces", :action => "icon", :id => trace.id, :display_name => trace.user.display_name %>" alt="" class="img-thumbnail mr-3" />
+      </a>
+    <% else %>
+      <div style="width: 60px; height: 60px" class="img-thumbnail bg-white mr-3"></div>
+    <% end %>
+  <% end %>
+
+  <div class="media-body">

The media component has been removed from bootstrap 5. See https://github.com/twbs/bootstrap/pull/28265. So to avoid having to refactor this soon, we should avoid using it now.

> @@ -1,35 +1,44 @@
-<tr>
-  <td>
-    <% if Settings.status != "gpx_offline" %>
-      <% if trace.inserted %>
-        <a href="<%= url_for :controller => "traces", :action => "show", :id => trace.id, :display_name => trace.user.display_name %>"><img src="<%= url_for :controller => "traces", :action => "icon", :id => trace.id, :display_name => trace.user.display_name %>" border="0" alt="" /></a>
-      <% else %>
-        <span class="text-danger"><%= t ".pending" %></span>
-      <% end %>
+<li class="media mb-1 p-2 <%= cycle("", " bg-light") %>">

I count 10 different uses of margin and padding utilities - seems a bit high? If there is any way to reduce them, that would make the layout more robust to future changes. In particular, I can't see any system behind choosing different sizes e.g. `-1`, `-2` etc.

> @@ -1,35 +1,44 @@
-<tr>
-  <td>
-    <% if Settings.status != "gpx_offline" %>
-      <% if trace.inserted %>
-        <a href="<%= url_for :controller => "traces", :action => "show", :id => trace.id, :display_name => trace.user.display_name %>"><img src="<%= url_for :controller => "traces", :action => "icon", :id => trace.id, :display_name => trace.user.display_name %>" border="0" alt="" /></a>
-      <% else %>
-        <span class="text-danger"><%= t ".pending" %></span>
-      <% end %>
+<li class="media mb-1 p-2 <%= cycle("", " bg-light") %>">

We recently moved away from `cycle` in templates, although this is less intrusive than what we had previously. See https://github.com/openstreetmap/openstreetmap-website/commit/5fdada204cf92d8ca99300927713724aaaf1b2bd for the change.

This proposes changing the colour for the stripes, which makes it inconsistent with the other striped tables/lists e.g. messages.

Is there a different way to achieve this? Or alternatively, does writing more CSS to support attempting to stripe a list suggest that we should be sticking with a table?

> -<tr>
-  <td>
-    <% if Settings.status != "gpx_offline" %>
-      <% if trace.inserted %>
-        <a href="<%= url_for :controller => "traces", :action => "show", :id => trace.id, :display_name => trace.user.display_name %>"><img src="<%= url_for :controller => "traces", :action => "icon", :id => trace.id, :display_name => trace.user.display_name %>" border="0" alt="" /></a>
-      <% else %>
-        <span class="text-danger"><%= t ".pending" %></span>
-      <% end %>
+<li class="media mb-1 p-2 <%= cycle("", " bg-light") %>">
+  <% if Settings.status != "gpx_offline" %>
+    <% if trace.inserted %>
+      <a href="<%= url_for :controller => "traces", :action => "show", :id => trace.id, :display_name => trace.user.display_name %>">
+        <img src="<%= url_for :controller => "traces", :action => "icon", :id => trace.id, :display_name => trace.user.display_name %>" alt="" class="img-thumbnail mr-3" />
+      </a>
+    <% else %>
+      <div style="width: 60px; height: 60px" class="img-thumbnail bg-white mr-3"></div>

I don't think our CSP allows inline styling like this. But if we're making empty divs a specific width and height, to push other content around, then we should be looking at using a grid or table instead.

> -        <span class="text-danger"><%= t ".pending" %></span>
-      <% end %>
+<li class="media mb-1 p-2 <%= cycle("", " bg-light") %>">
+  <% if Settings.status != "gpx_offline" %>
+    <% if trace.inserted %>
+      <a href="<%= url_for :controller => "traces", :action => "show", :id => trace.id, :display_name => trace.user.display_name %>">
+        <img src="<%= url_for :controller => "traces", :action => "icon", :id => trace.id, :display_name => trace.user.display_name %>" alt="" class="img-thumbnail mr-3" />
+      </a>
+    <% else %>
+      <div style="width: 60px; height: 60px" class="img-thumbnail bg-white mr-3"></div>
+    <% end %>
+  <% end %>
+
+  <div class="media-body">
+    <% if trace.inserted %>
+      <p class="float-right">

I'm not sure if floating is the right approach here, but I haven't investigated alternatives like grid.

>        <% end %>
-      ... <%= time_ago_in_words(trace.timestamp, :scope => :'datetime.distance_in_words_ago') %></span>
-      <%= link_to_if trace.inserted?, t(".map"), { :controller => "site", :action => "index", :mlat => trace.latitude, :mlon => trace.longitude, :anchor => "map=14/#{trace.latitude}/#{trace.longitude}" }, { :title => t(".view_map") } %> /
-      <%= link_to t(".edit"), { :controller => "site", :action => "edit", :gpx => trace.id }, { :title => t(".edit_map") } %>
+      <% badge_class = trace.visibility.in?("public", "identifiable") ? "success" : "danger" %>

This needs to be `in?(Array)` i.e. `trace.visibility.in?(["public", "identifiable"])` (note added `[ ]`)

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3036#pullrequestreview-562896001
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20210106/fd45b796/attachment.htm>