-
Notifications
You must be signed in to change notification settings - Fork 6
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🗑️ [#2062] Remove dashboard templatetags #1051
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1051 +/- ##
===========================================
- Coverage 94.92% 94.92% -0.01%
===========================================
Files 882 880 -2
Lines 30863 30841 -22
===========================================
- Hits 29298 29276 -22
Misses 1565 1565 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small point about the use of aria-label
in one place. Not sure if this needs to be addressed, so feel free to adopt or ignore as you see fit.
<aside class="faq" aria-label="{% trans "Veelgestelde vragen" %}"> | ||
{% if title %}<h2 id="faq" class="h2">{{ title }}</h2>{% endif %} | ||
{% trans "Veelgestelde vragen" as default_title %} | ||
<aside class="faq" aria-label="{{ default_title }}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a point for the aria-label
? Either the h2
is visible, then the content is accessible to screen readers, or the h2
is hidden, but then the h1
element from pages/faq.html
is visible and contains {% trans "Veelgestelde vragen" %}
, so again the content is accessible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm not sure, @jiromaykin can we omit the aria-label
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aria-label
on an 'aside' has very mixed support and here it seems redundant
No description provided.