[openstreetmap/openstreetmap-website] Adjust Bootstrap icon usage (PR #6501)

[Pablo Brasero]

@pablobm commented on this pull request.

Thank you for this, it's underappreciated, mind-numbing work! 💪 Left some comments regarding accessibility nuances, let's see what you think.

>                <span class="visually-hidden"><%= t ".title_label" %></span>
               <%= link_to entry.title, diary_entry_path(@user, entry), :class => "align-self-center lh-sm" %>
             </p>
             <p class="card-text flex-grow-1"><%= truncate(strip_tags(entry.body.to_html), :length => 150) %></p>
 
             <div class="card-text d-flex justify-content-between align-items-center mt-auto">
               <small class="text-body-secondary d-flex gap-1 align-items-center">
-                <i class="bi bi-chat fs-5 my-n1" aria-hidden="true"></i>
+                <i class="bi bi-chat fs-5 my-n1" title="<%= t ".comments_label" %>" aria-hidden="true"></i>

Is this specific `title` needed? Also the `span` right after. They are followed by an "X comments"  line, so I feel they might be redundant.

>                  <span class="visually-hidden"><%= t ".comments_label" %></span>
                 <%= link_to t(".comments", :count => entry.comments.size), diary_entry_path(@user, entry, :anchor => "comments"), :class => "text-body-secondary" %>
               </small>
               <small class="text-body-secondary d-flex gap-1 align-items-center">
-                <i class="bi bi-calendar fs-5 my-n1" aria-hidden="true"></i>
+                <i class="bi bi-calendar fs-5 my-n1" title="<%= t ".date_label" %>" aria-hidden="true"></i>

Similarly here: perhaps the `title` and `span` are not necessary as the content is self-explanatory.

> +        <i class="bi bi-star-fill fs-5 role-<%= role %> align-middle" title="<%= t ".revoke.#{role}" %>" aria-hidden="true"></i>
+        <span class="visually-hidden"><%= t ".revoke.#{role}" %></span>

Related to the above: do we need both the `title` and the `span`? I suspect a single one will suffice. As for which one... any a11y experts in the room? 😅 

> +        <i class="bi bi-star fs-5 role-<%= role %> align-middle" title="<%= t ".grant.#{role}" %>" aria-hidden="true"></i>
+        <span class="visually-hidden"><%= t ".grant.#{role}" %></span>

Same here.

> +          <i class="align-self-baseline bi bi-geo-alt-fill flex-shrink-0 fs-6" title="<%= t ".home_location" %>" aria-hidden="true"></i>
           <span class="visually-hidden"><%= t ".home_location" %></span>

Same: probably one is enough.

> +          <i class="align-self-baseline bi bi-buildings-fill flex-shrink-0 fs-6" title="<%= t ".company" %>" aria-hidden="true"></i>
           <span class="visually-hidden"><%= t ".company" %></span>

And same.

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

Message ID: <openstreetmap/openstreetmap-website/pull/6501/review/3427773314 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20251106/7ace70fa/attachment.htm>