Skip to content

LG-7150 add login icon alt text#7023

Merged
svalexander merged 7 commits intomainfrom
shannon/lg-7150-login-icon-alt-text
Sep 26, 2022
Merged

LG-7150 add login icon alt text#7023
svalexander merged 7 commits intomainfrom
shannon/lg-7150-login-icon-alt-text

Conversation

@svalexander
Copy link
Contributor

@svalexander svalexander commented Sep 23, 2022

Ticket

What: This pr adds alt text to the Login.gov logo in order to allow screen reader users to hear text that describes the logo image in emails.

How: Alt tag added to image_tag in the base email used by other emails.
Note: Did not add this text to the i18l files as the text is the same in the 3 languages we support.

Testing:

  • Go to one of the emails, can use ready to verify email for example. http://localhost:3000/rails/mailers/user_mailer/in_person_verified.html?locale=en
  • Open finder
  • Navigate to VoiceOver and enable
  • Navigate the email using VoiceOver to ensure that "Login.gov logo" is read out when highlighting the logo
Screenshot Screen Shot 2022-09-22 at 4 56 42 PM

changelog: Improvement, Accessibility, add alt text for login.gov logo

@svalexander svalexander changed the title Shannon/lg 7150 login icon alt text lg-7150 add login icon alt text Sep 23, 2022
attachments['logo.png'].url,
size: '142x19',
style: 'width: 142px; height: 19px;',
alt: 'Login.gov logo',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we conventionally use the APP_NAME constant for this rather than hardcoding Login.gov.

Suggested change
alt: 'Login.gov logo',
alt: "#{APP_NAME} logo",

I noticed that the only place this is hardcoded are the error pages.

@aduth
Copy link
Contributor

aduth commented Sep 26, 2022

Note: Did not add this text to the i18l files as the text is the same in the 3 languages we support.

I think we should still use the localization tools even if the text is the same, in case we need to make revisions to one language in the future, and to prevent confusion if a future maintainer assumes that this text is untranslated.

@svalexander
Copy link
Contributor Author

that makes sense, i just updated it

@aduth
Copy link
Contributor

aduth commented Sep 26, 2022

For the build failure, you can bypass the error by adding to the list here as being the same translation across all locales:

# List of keys allowed to be untranslated or are the same as English
ALLOWED_UNTRANSLATED_KEYS = [
{ key: 'account.navigation.menu', locales: %i[fr] }, # "Menu" is "Menu" in French
{ key: 'doc_auth.headings.photo', locales: %i[fr] }, # "Photo" is "Photo" in French
{ key: 'components.status_page.icons.error', locales: %i[es] }, # "Error" is "Error" in Spanish
{ key: 'components.status_page.icons.question', locales: %i[fr] }, # "Question" is "Question" in French
{ key: 'errors.alt.error', locales: %i[es] }, # "Error" is "Error" in Spanish
{ key: /^i18n\.locale\./ }, # Show locale options translated as that language
{ key: /^countries/ }, # Some countries have the same name across languages
{ key: 'links.contact', locales: %i[fr] }, # "Contact" is "Contact" in French
{ key: 'simple_form.no', locales: %i[es] }, # "No" is "No" in Spanish
{ key: 'simple_form.required.html' }, # No text content
{ key: 'simple_form.required.mark' }, # No text content
{ key: 'time.am' }, # "AM" is "AM" in French and Spanish
{ key: 'time.pm' }, # "PM" is "PM" in French and Spanish
{ key: 'datetime.dotiw.minutes.one' }, # "minute is minute" in French and English
{ key: 'datetime.dotiw.minutes.other' }, # "minute is minute" in French and English
].freeze

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

I see you added "[skip changelog]", but this seems like something which would be good to highlight in the release notes as an accessibility improvement / fix. You could still add a changelog as part of the editable textarea when merging.

end

it 'includes alt text for app logo that reads Login.gov logo' do
expect(rendered).to have_css("img[alt='Login.gov logo']")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this probably passes, it'd be good to make this assertion locale-independent.

Suggested change
expect(rendered).to have_css("img[alt='Login.gov logo']")
expect(rendered).to have_css("img[alt='#{t('mailer.logo', app_name: APP_NAME)}']")

@svalexander svalexander merged commit a49111a into main Sep 26, 2022
@svalexander svalexander deleted the shannon/lg-7150-login-icon-alt-text branch September 26, 2022 20:00
@svalexander svalexander changed the title lg-7150 add login icon alt text LG-7150 add login icon alt text May 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants