LG-7242 Accessible Icons by eric-gade · Pull Request #6983 · 18F/identity-idp

eric-gade · 2022-09-19T14:01:49Z

What

This PR replaces some of the current iconography with better, more accessible versions. We also update the templates in which these icons are used so that they specify better dimensions for the new designs.

Will need to update where it gets used, since this appears to be the first instance of the icon

-- What We have replaced half a dozen existing icons with new SVGs. These have different names in some cases, so the references need to be replaced in templates. Additionally, these icons require larger display sizes given their circular shape, so we need to find corresponding template tags and make sure they are given the correct dimensions (88x88px) changelog: Internal, Assets, updating icon images and references

changelog: Internal, Assets, updating icon images and references

changelog: Internal, Accessibility, updating icons

changelog: Internal, Accessibility, updating accessible icons

app/assets/stylesheets/components/_modal.scss

app/views/account_reset/confirm_delete_account/show.html.erb

changelog: Internal, Accessibility, updating to more accessible icons

changelog: Internal, Accessibility, switching to more accessible icons

zachmargolis

LGTM, might be good to add some before/after screenshots to the PR, or maybe some URLs/screens

app/assets/stylesheets/components/_modal.scss

app/assets/stylesheets/components/_personal-key.scss

app/assets/stylesheets/components/_modal.scss

aduth · 2022-09-21T13:07:26Z

app/assets/stylesheets/components/_personal-key.scss

@@ -2,7 +2,7 @@
  position: relative;

  &::before {


Is this a duplicate style from the other in _modal.scss. I see both are being applied to the element, it seems like we should only have one.

Also updating any corresponding templates to use the new localizations. changelog: Internal, Accessibility, updating to more accessible icons

changelog: Internal, Accessibility, adding more accessible icons

changelog: Internal, Accessibility, updating to more accessible icons

eric-gade · 2022-09-21T21:02:36Z

Based on the previous comments by @aduth , and noticing how the modals are implemented throughout the application currently, I have made some slight but wide reaching changes to how icons are displayed.

Recent commits include the following changes:

width and height are no longer specified in template markup for icon images. Instead we use the class lg-icon. The idea is that if we need different icon sizes in the future, we can add appropriate classes.
The "badge" that displays at the top of a modal is now described in the _icons.scss file, and has been fixed to display correctly and without explicit positioning numbers.
I've added the correct alt text and localizations for each of the new icons, and applied those localizations wherever the icons have been used

changelog: Internal, Accessibility, adding more accessible icons

eric-gade · 2022-09-21T21:51:43Z

Some Before and Afters

Could Not Verify ID

BEFORE

AFTER

Confirm Personal Key

BEFORE

AFTER

Don't Have Your Personal Key?

BEFORE

AFTER

Account Deleted

BEFORE

AFTER

changelog: Internal, Accessibility, adding more accessible icons

app/javascript/packages/components/status-page.tsx

config/locales/titles/en.yml

app/assets/stylesheets/components/_icon.scss

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

Also updating the signature of the initialization method changelog: Internal, Accessibility, adding more accessible icons

changelog: Internal, Accessibility, adding more accessible icons

Also updating and corresponding location references. changelog: Internal, Accessibility, adding more accessible icons

aduth · 2022-09-26T17:49:22Z

app/views/sign_up/registrations/_required_pii_accordion.html.erb

-          size: '40', alt: t('devise.registrations.start.bullet_6_img'), class: 'margin-top-05 grid-col-auto',
+    <%= render AlertIconComponent.new(
+          icon_name: :personal_key,
+          size: '40', alt: t('devise.registrations.start.bullet_6_img'), class: 'margin-top-05 grid-col-auto'


Based on the failing build, you'll either need to remove, update, or account for the size attribute here, since it's incompatible with the AlertIconComponent's own default width and height.

Suggested change

size: '40', alt: t('devise.registrations.start.bullet_6_img'), class: 'margin-top-05 grid-col-auto'

width: 40,

height: 40,

alt: t('devise.registrations.start.bullet_6_img'),

class: 'margin-top-05 grid-col-auto',

Or something like this in AlertIconComponent:

def size_attributes if tag_options[:size].present? { size: tag_options[:size] } else { width: DEFAULT_WIDTH, height: DEFAULT_HEIGHT } end end

aduth · 2022-09-26T17:54:23Z

app/views/sign_up/registrations/_required_pii_accordion.html.erb

-    <%= image_tag(
-          asset_url('p-key.svg'),
-          size: '40', alt: t('devise.registrations.start.bullet_6_img'), class: 'margin-top-05 grid-col-auto',
+    <%= render AlertIconComponent.new(


On second thought, do we actually want to be updating this icon? It will introduce inconsistency with the rest in the context it's shown (the sign in page when authenticating with an SP at IAL2).

I see now. The p-key.svg that was hanging out at the root level (which I deleted) should have been in get-started with the rest of them. Restoring now.

Old screen (now restored)

It's kinda strange to me that the icon for personal key is red when the rest are blue, but I'm assuming it's always been that way, so I think it's something to follow-up with separately.

aduth · 2022-09-26T17:55:03Z

app/views/users/backup_code_setup/confirm_delete.html.erb

 <% title t('forms.backup_code.confirm_delete') %>

-<%= image_tag(asset_url('status/warning.svg'), alt: t('errors.alt.warning'), width: 54, class: 'display-block margin-bottom-4') %>
+<%= render AlertIconComponent.new(icon_name: :warning, class: 'display-block margin-bottom-4 alert-icon') %>


The 'alert-class' class would be added by the component itself, so I wouldn't expect to see it here.

Suggested change

<%= render AlertIconComponent.new(icon_name: :warning, class: 'display-block margin-bottom-4 alert-icon') %>

<%= render AlertIconComponent.new(icon_name: :warning, class: 'display-block margin-bottom-4') %>

changelog: Internal, Accessibility, adding more accessible icons

Also reverting to plain image_tag for the rendering, as with other sibling icon images in the partial changelog: Internal, Accessibility, adding more accessible icons

changelog: Internal, Accessibility, adding more accessible icons

aduth

Few more comments, but nothing blocking. LGTM 👍

aduth · 2022-09-26T19:58:19Z

config/locales/image_description/en.yml

 en:
  image_description:
    camera_mobile_phone: Camera flashing on a mobile phone
+    delete: Red trash can


I'd have a light preference to have these strings at components.alert_icon to have a clear and consolidated source for component strings. Not a blocker though.

app/components/alert_icon_component.html.erb

app/components/alert_icon_component.rb

aduth · 2022-09-26T20:19:57Z

app/javascript/packages/components/status-page.tsx

+        alt={alt}
+        width={88}
+        height={88}
+        className="display-block margin-bottom-4 alert-icon"


Would be good to follow-up to align the componentization of AlertIcon in React to match the corresponding ViewComponent implementation.

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

changelog: Internal, Accessibility, adding more accessible icons

…lg-7242

We are also adding a couple of tests to ensure that AlertIconComponent is correctly handling the logic if width, height, and size attributes changelog: Internal, Accessibility, adding more accessible icons

changelog: Internal, Accessibility, adding more accessible icons

spec/components/alert_icon_component_spec.rb

changelog: Internal, Accessibility, adding more accessible icons

It's not currently being used, so we are removing any handling of this option changelog: Internal, Accessibility, adding more accessible icons

kbighorse · 2022-09-27T17:58:59Z

app/components/status_page_component.rb

  def initialize(status: :error, icon: nil)
-    if !ICONS.key?(status)
-      raise ArgumentError, "`status` #{status} is invalid, expected one of #{ICONS.keys}"
+    if !VALID_STATUS.include?(status)


[comment] prefer if with a positive test, or consider unless in a guard clause, e.g. raise ... unless VALID_STATUS.include?(status)

kbighorse · 2022-09-27T18:01:05Z

app/views/shared/_personal_key_confirmation_modal.html.erb

 <div id="personal-key-confirm" class="display-none" tabindex='-1'>
  <%= render layout: '/shared/modal_layout', locals: { role: 'dialog' } do |label_id, description_id| %>
-    <div class="padding-y-8 padding-x-2 tablet:padding-x-8 cntnr-skinny bg-white radius-lg key-badge">
+      <div class="padding-y-8 padding-x-2 tablet:padding-x-8 cntnr-skinny bg-white radius-lg iconic-modal-badge personal-key-badge">


is this indentation intentional?

kbighorse · 2022-09-27T18:02:53Z

app/views/users/webauthn_setup/delete.html.erb

@@ -1,10 +1,10 @@
 <% if @webauthn.platform_authenticator %>
-  <% title t('forms.webauthn_platform_delete.confirm') %>
+    <% title t('forms.webauthn_platform_delete.confirm') %>


indentation here?

kbighorse · 2022-09-27T18:03:01Z

app/views/users/webauthn_setup/delete.html.erb

+    <% title t('forms.webauthn_platform_delete.confirm') %>
 <% else %>
-  <% title t('forms.webauthn_delete.confirm') %>
+    <% title t('forms.webauthn_delete.confirm') %>


indentation here?

kbighorse · 2022-09-27T18:08:46Z

spec/views/idv/shared/_error.html.erb_spec.rb


      it 'includes informative image' do
-        expect(rendered).to have_css("[src*='warning'][alt=#{t('errors.alt.warning')}]")
+        expect(rendered).to have_css("[src*='warning'][alt='#{t('image_description.warning')}']")


[suggestion] consider using a helper, if possible, for this interpolation

kbighorse · 2022-09-27T18:08:52Z

spec/views/idv/shared/_error.html.erb_spec.rb


      it 'includes informative image' do
-        expect(rendered).to have_css("[src*='error'][alt=#{t('errors.alt.error')}]")
+        expect(rendered).to have_css("[src*='error'][alt='#{t('image_description.error')}']")


[suggestion] consider using a helper, if possible, for this interpolation

kbighorse

Made some non-blocking comments. 👍🏾

eric-gade added 8 commits September 13, 2022 15:07

Updating all warning icon images to new icon

61729c8

Updating error icon

74e1031

Adding personal-key icon

6d90dfd

Will need to update where it gets used, since this appears to be the first instance of the icon

Updating info-question icon

38d1fc3

Replacing error-lock icon

63d9b67

Updating icon image dimensions in templates

7aba3eb

changelog: Internal, Assets, updating icon images and references

Updating remaining templates with new icons sizes

1058b42

changelog: Internal, Accessibility, updating icons

eric-gade requested a review from a team September 19, 2022 14:02

Updating SVG assets to not have inline styles

f41b578

changelog: Internal, Accessibility, updating accessible icons

aduth reviewed Sep 19, 2022

View reviewed changes

app/assets/stylesheets/components/_modal.scss Show resolved Hide resolved

aduth reviewed Sep 19, 2022

View reviewed changes

app/views/account_reset/confirm_delete_account/show.html.erb Outdated Show resolved Hide resolved

eric-gade added 3 commits September 19, 2022 12:18

Removing now unused image from assets

cf6cb0e

changelog: Internal, Accessibility, updating to more accessible icons

Updating alt test for confirm delete account icon

bbf7df2

changelog: Internal, Accessibility, updating to more accessible icons

Fixing lints

be77737

changelog: Internal, Accessibility, switching to more accessible icons

zachmargolis approved these changes Sep 20, 2022

View reviewed changes

aduth reviewed Sep 21, 2022

View reviewed changes

eric-gade added status - work in progress status - do not merge labels Sep 21, 2022

eric-gade added 5 commits September 21, 2022 15:16

Adding localized alt text for new icons

eebde06

Also updating any corresponding templates to use the new localizations. changelog: Internal, Accessibility, updating to more accessible icons

Fixing es local yaml typo

3432e8b

Updating templates and modals to use new icon css

cab93ce

changelog: Internal, Accessibility, adding more accessible icons

Updating status page component with new icon locales

a5c0153

changelog: Internal, Accessibility, adding more accessible icons

Normalizing the yaml

6f7118c

changelog: Internal, Accessibility, updating to more accessible icons

Removing old icon css from _modal

8e6de7c

changelog: Internal, Accessibility, adding more accessible icons

Fixing dont have personal key modal styling

6f95634

changelog: Internal, Accessibility, adding more accessible icons

aduth reviewed Sep 22, 2022

View reviewed changes

Update app/assets/stylesheets/components/_icon.scss

6af36d1

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

eric-gade added 7 commits September 26, 2022 12:15

Ensuring that we raise an error if invalid icon_name is passed

273d30d

Also updating the signature of the initialization method changelog: Internal, Accessibility, adding more accessible icons

Updating views to use new AlertIconComponent initializer

e5e93b5

changelog: Internal, Accessibility, adding more accessible icons

Simplifying custom tag options for alt text

92e757c

changelog: Internal, Accessibility, adding more accessible icons

Ensure we are using symbols for icon names

fcdd97a

changelog: Internal, Accessibility, adding more accessible icons

fixing lints

f162fe0

changelog: Internal, Accessibility, adding more accessible icons

Fixing initializer of AlertIconComponent

2534a5d

changelog: Internal, Accessibility, adding more accessible icons

Moving personal key icon image to status

ed2acb8

Also updating and corresponding location references. changelog: Internal, Accessibility, adding more accessible icons

aduth reviewed Sep 26, 2022

View reviewed changes

eric-gade added 3 commits September 26, 2022 14:54

Removing redundant alert-icon class declaration

c3d5421

changelog: Internal, Accessibility, adding more accessible icons

Restoring get-started accordion personal key image

a2e5969

Also reverting to plain image_tag for the rendering, as with other sibling icon images in the partial changelog: Internal, Accessibility, adding more accessible icons

Accounting for possible size tag option in component template

fe5d13d

changelog: Internal, Accessibility, adding more accessible icons

aduth approved these changes Sep 26, 2022

View reviewed changes

eric-gade and others added 5 commits September 26, 2022 16:32

Update app/components/alert_icon_component.rb

a79e7df

Co-authored-by: Andrew Duthie <andrew.duthie@gsa.gov>

Fixing conflicting linter errors

7d60113

changelog: Internal, Accessibility, adding more accessible icons

Merge branch 'eric-lg-7242' of github-tts:18F/identity-idp into eric-…

4d9502a

…lg-7242

Refactoring width, height, and size interplay

5ee77b2

We are also adding a couple of tests to ensure that AlertIconComponent is correctly handling the logic if width, height, and size attributes changelog: Internal, Accessibility, adding more accessible icons

Empty commit for gitlab CI

35a9695

changelog: Internal, Accessibility, adding more accessible icons

aduth reviewed Sep 27, 2022

View reviewed changes

spec/components/alert_icon_component_spec.rb Outdated Show resolved Hide resolved

spec/components/alert_icon_component_spec.rb Outdated Show resolved Hide resolved

eric-gade added 2 commits September 27, 2022 09:55

Cleaning up tests

27c07a4

changelog: Internal, Accessibility, adding more accessible icons

Removing casing for size option

5abe548

It's not currently being used, so we are removing any handling of this option changelog: Internal, Accessibility, adding more accessible icons

kbighorse reviewed Sep 27, 2022

View reviewed changes

kbighorse approved these changes Sep 27, 2022

View reviewed changes

eric-gade merged commit 8c3688d into main Sep 27, 2022

eric-gade deleted the eric-lg-7242 branch September 27, 2022 18:21

solipet mentioned this pull request Sep 29, 2022

Deploy RC 213 to Prod #7054

Merged

	<%= render AlertIconComponent.new(icon_name: :warning, class: 'display-block margin-bottom-4 alert-icon') %>
	<%= render AlertIconComponent.new(icon_name: :warning, class: 'display-block margin-bottom-4') %>

Conversation

eric-gade commented Sep 19, 2022

What

Uh oh!

Uh oh!

Uh oh!

zachmargolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eric-gade commented Sep 21, 2022

Uh oh!

eric-gade commented Sep 21, 2022

Some Before and Afters

Could Not Verify ID

Confirm Personal Key

Don't Have Your Personal Key?

Account Deleted

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aduth Sep 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aduth left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kbighorse left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aduth Sep 26, 2022 •

edited

Loading