<p></p>
<p><b>@gravitystorm</b> requested changes on this pull request.</p>
<p>Overall I like parts of this PR, e.g. removing the <code>...</code> and making the link text clearer. But I'm not convinced by the list vs tables - not yet, but still a possibility!</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3036#discussion_r552828917">app/views/traces/_trace.html.erb</a>:</p>
<pre style='color:#555'>> - <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">
</pre>
<p>The media component has been removed from bootstrap 5. See <a class="issue-link js-issue-link" data-error-text="Failed to load title" data-id="410420406" data-permission-text="Title is private" data-url="https://github.com/twbs/bootstrap/issues/28265" data-hovercard-type="pull_request" data-hovercard-url="/twbs/bootstrap/pull/28265/hovercard" href="https://github.com/twbs/bootstrap/pull/28265">twbs/bootstrap#28265</a>. So to avoid having to refactor this soon, we should avoid using it now.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3036#discussion_r552833337">app/views/traces/_trace.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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") %>">
</pre>
<p>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. <code>-1</code>, <code>-2</code> etc.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3036#discussion_r552837515">app/views/traces/_trace.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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") %>">
</pre>
<p>We recently moved away from <code>cycle</code> in templates, although this is less intrusive than what we had previously. See <a class="commit-link" data-hovercard-type="commit" data-hovercard-url="https://github.com/openstreetmap/openstreetmap-website/commit/5fdada204cf92d8ca99300927713724aaaf1b2bd/hovercard" href="https://github.com/openstreetmap/openstreetmap-website/commit/5fdada204cf92d8ca99300927713724aaaf1b2bd"><tt>5fdada2</tt></a> for the change.</p>
<p>This proposes changing the colour for the stripes, which makes it inconsistent with the other striped tables/lists e.g. messages.</p>
<p>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?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3036#discussion_r552838713">app/views/traces/_trace.html.erb</a>:</p>
<pre style='color:#555'>> -<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>
</pre>
<p>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.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3036#discussion_r552839937">app/views/traces/_trace.html.erb</a>:</p>
<pre style='color:#555'>> - <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">
</pre>
<p>I'm not sure if floating is the right approach here, but I haven't investigated alternatives like grid.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3036#discussion_r552841604">app/views/traces/_trace.html.erb</a>:</p>
<pre style='color:#555'>> <% 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" %>
</pre>
<p>This needs to be <code>in?(Array)</code> i.e. <code>trace.visibility.in?(["public", "identifiable"])</code> (note added <code>[ ]</code>)</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />You are receiving this because you are subscribed to this thread.<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/3036#pullrequestreview-562896001">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLOSF5WOVXU3XKGRDSDSYSNSVANCNFSM4VSFYJGQ">unsubscribe</a>.<img src="https://github.com/notifications/beacon/AAK2OLJRY75HAJA5NR7IHL3SYSNSVA5CNFSM4VSFYJG2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOEGGRZAI.gif" height="1" width="1" alt="" /></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/3036#pullrequestreview-562896001",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/3036#pullrequestreview-562896001",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>