Traces: Migrate list to bootstrap #3036
Conversation
- switch from tables to bootstrap media since it makes the spacing around the image easier and also its not a table in a semantic way - remove the unless in #index since it is covered by the if-size>0 above refactor the list - reorder elements - make the pending-image a blank square and move the pending notice to the first line - hide both actions if trace is pending - move the actions to the right and use the previous title-text as link-text since those explain a lot better what the links actually do (I thought "edit" would edit the trace-db-entry, not open the map-editor) - remove the two translations that are not used anymore from en.yml - use if-present for tags-link-list since its easier to read - use the same styling for the trace description as on the changeset description (italic text)
Solving linter messages. ``` 4 error(s) were found in ERB files Linting 154 files with 12 linters... Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. In file: app/views/traces/_trace.html.erb:1 Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping. In file: app/views/traces/_trace.html.erb:1 Layout/CaseIndentation: Indent `when` as deep as `case`. In file: app/views/traces/_trace.html.erb:30 Layout/EndAlignment: `end` at 4, 24 is not aligned with `case` at 1, 23. In file: app/views/traces/_trace.html.erb:32 ```
| <% if trace.tags.present? %> | ||
| <%= t ".in" %> | ||
| <%= safe_join(trace.tags.collect { |tag| link_to_tag tag.tag }, ", ") %> | ||
| <% end %> |
There was a problem hiding this comment.
This bit here looks like a Lego-style translation, as it translates the text word by word. Obviously I can't tell if that's causing some issue for any of the ~97 languages we support.
Typically you would define one locale string that includes multiple variables and let the translators figure out what the proper text would be in their language.
There was a problem hiding this comment.
I try not to touch the output – translations and things like this join – in all my PRs since my focus is on refactoring the html/css to bootstrap and while doing this moving the elements to a – in my opinion – better place in the view. I hope this will make it easier to merge since I expect less discussion about the right wording and less re-translating needs to be done.
If we where to change this specific translation, I would vote for changing the meaning of "in" to something like "categorised with" or "tagged with". The "in" term does not make much sense IMO, since the tags are not the main "folder" where the traces are "in", nor are the tags the main navigation element on the traces page.
There was a problem hiding this comment.
Yes that's a separate issue and should be addressed (if there is indeed a problem) in a separate PR.
| <% end %> | ||
| <% end %> | ||
|
|
||
| <div class="media-body"> |
There was a problem hiding this comment.
The media component has been removed from bootstrap 5. See twbs/bootstrap#28265. So to avoid having to refactor this soon, we should avoid using it now.
| <% else %> | ||
| <span class="text-danger"><%= t ".pending" %></span> | ||
| <% end %> | ||
| <li class="media mb-1 p-2 <%= cycle("", " bg-light") %>"> |
There was a problem hiding this comment.
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.
| <% else %> | ||
| <span class="text-danger"><%= t ".pending" %></span> | ||
| <% end %> | ||
| <li class="media mb-1 p-2 <%= cycle("", " bg-light") %>"> |
There was a problem hiding this comment.
We recently moved away from cycle in templates, although this is less intrusive than what we had previously. See 5fdada2 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?
| <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> |
There was a problem hiding this comment.
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.
|
|
||
| <div class="media-body"> | ||
| <% if trace.inserted %> | ||
| <p class="float-right"> |
There was a problem hiding this comment.
I'm not sure if floating is the right approach here, but I haven't investigated alternatives like grid.
| ... <%= 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" %> |
There was a problem hiding this comment.
This needs to be in?(Array) i.e. trace.visibility.in?(["public", "identifiable"]) (note added [ ])
gravitystorm
added
the
changes requested
Ideas taken from openstreetmap#3036
Ideas taken from openstreetmap#3036
|
I've refactored this based on the review notes, and opened a new PR at #3139 |
Changes:
refactor the list
I will wait for a first review of this.
Still to do:
Screenshots
BEFORE
AFTER
AFTER / PENDING
