@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/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
