a11y: update credit link markup for screen-readers#5106
Conversation
joemull
left a comment
joemull
left a comment
There was a problem hiding this comment.
Thanks for catching these things.
I've got a couple of suggestions for how to simplify it, and there's also a bug in the double for-loop.
| {{ record }}{% if not forloop.last %}, {% endif %} | ||
| {% if journal_settings.general.use_credit %} | ||
| {% for author_check in article.frozen_authors.all %} | ||
| {% if author_check.credits.exists %} |
There was a problem hiding this comment.
This creates two levels of for-loops, causing the section to display multiple times, once for each author:
I'd probably fix it instead by creating a new context variable called credit_records_exist in the view, which queries credit records for a any relationship to the article, like this:
credit_records_exist = models.CreditRecord.objects.filter(frozen_author__article=article).exists()Then use that instead of the broken piece of logic:
{% if journal_settings.general.use_credit and credit_records_exist %}There was a problem hiding this comment.
Logic updated as suggested.
| <a href="https://www.niso.org/publications/z39104-2022-credit" target="_blank"> | ||
| <span class="sr-only">{% trans "Read more about Credit." %} </span> | ||
| <i aria-hidden="true" class="fa fa-info"></i> | ||
| {% include "elements/icons/link_external.html" %} |
There was a problem hiding this comment.
This feels a bit too icon-y, and the two icons right next to each other are visually distracting, since they are not positioned the same way (I understand why, but nevertheless).
Maybe we can just get rid of the i and replace it with the phrase "read more", with the whole link in parentheses? This would make the external link icon work better.
There was a problem hiding this comment.
I'd much prefer that!
There was a problem hiding this comment.
And Read more link. It looked intrusive under the heading, so I put it at the end intead.
|
Note to self - update is now working on OLH, but credit no longer displaying on Clean or Material, can't find it on Master either, need more investigation as to what is going wrong. As I only started that bugfix because it was working on C & M and not on O. |
3 participants
single instance of non-compliance with WCAG 1.3.3 (#5105) for the OLH theme.
the audit for this SC identified that the icon had no text-equivalent. Then while fixing, I also found the external link wasn't marked up so added that fix which would otherwise have been found on a later audit when looking at navigation.
While fixing this, I also found that CrediT wasn't displaying on the OLH article page at all, due to a logic error. I have fixed that in the second commit here.