-
Notifications
You must be signed in to change notification settings - Fork 3
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
El 1975 hook up translation link #429
base: main
Are you sure you want to change the base?
Conversation
c3ec7de
to
6ff2840
Compare
146bc3b
to
2f59850
Compare
e210460
to
711624e
Compare
a86092e
to
b94213c
Compare
and remove inline javascript
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.
LGTM - feels more straight forward than I was expecting.
fala/common/mixin_for_views.py
Outdated
def translation_link(self, request): | ||
user_language = request.COOKIES.get("django_language", "en") | ||
if user_language == "cy": | ||
return "<a class='govuk-body govuk-link' id='language_switcher_link'>English</a> / Cymraeg" |
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.
I've spotted a small problem. These anchor links do not have href
. Because of that the link doesn't show a cursor icon when you hover over them.
An tag without an href attribute fails accessibility because it is not recognised as an interactive element by screen readers, keyboard navigation, and assistive technologies.
You will need to add href="#"
However the anchor link may try and go to the URL, so below in the JS I have added some code to help prevent that.
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.
You're also missing some other needed attributes
These are all on DWP component so you can find anything you've missed and need there.
hreflang="cy"
lang="cy"
rel="alternate"
aria-label="Newid yr iaith ir Gymraeg"
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.
👍 thanks @psweeting1 have made these updates. I think the form seems to work without the javascript to prevent the link behaviour but this is definitely worth testing.
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.
LGTM we just have to wait on the accessibility team to confirm the HTML.
fala/common/mixin_for_views.py
Outdated
if user_language == "cy": | ||
return "<a class='govuk-body govuk-link' id='language_switcher_link' href='#' hreflang='en' lang='en' rel='altternate' aria-label='English'>English</a> / Cymraeg" | ||
else: | ||
return "English / <a class='govuk-body govuk-link' id='language_switcher_link' href='#' hreflang='cy' lang='cy' rel='altternate' aria-label='Cymraeg'>Cymraeg</a>" |
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.
Can you use DWP's Aria label as they've got a really good descriptions for both welsh and english. aria-label
should always be descriptive and meaningful, providing enough context for screen readers to understand the purpose of the element.
Both english and welsh aria labels say "change language to X" on the DWP ones.
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.
👍 done
|
What does this pull request do?
Any other changes that would benefit highlighting?
Intentionally left blank.
Checklist