LG-8543 Accepted ID Alert by JackRyan1989 · Pull Request #7835 · 18F/identity-idp

JackRyan1989 · 2023-02-15T15:24:35Z

🎫 Ticket

🛠 Summary of changes

Added new alert component to /verify/in_person/state_id page
Added relevant text to en, es and fr translation files
Added small styling change for
📜 Testing Plan

Provide a checklist of steps to confirm the changes.
- Confirm that the alert appears on /verify/in_person/state_id page
- Confirm that relevant text appears for each translation
👀 Screenshots

Quick run thru with a screen reader:

After:

alert_sc_runthru.mov

…nslations.

…ecifying accepted forms of identification

allthesignals · 2023-02-15T15:36:30Z

app/assets/stylesheets/components/_alert.scss

+
+.usa-alert__body {
+  ul {
+    margin-bottom: 0px;


I'm not sure what the standard pattern is but do we use these USWDS class for things like this?

Yes I'd personally prefer that we use a margin utility class on the rendered list rather than making this default styling for all alerts.

i.e. <ul class="margin-bottom-0">

I don't think I understand the rails component system enough but is this going to affect other usages of usa-alert__body elsewhere?

Yes it would, since the stylesheets are loaded on every page.

A few thoughts:

Personally, I feel that alert content should be concise and we should try to avoid long text, particularly if it's going to be announced to assistive technology as status text. Regardless of whether we have the option here, my thinking is that we can disincentivize it by making it cumbersome to style long-form content 😅

There may be use-cases for lists followed by other content in an alert, where we would want the margin

Any additional styling to design system components beyond what's provided through the base design system could create confusion for developers who are familiar with USWDS styles but not our ad hoc extensions

Maybe it's reasonable to have a global styling to remove the margin from the last child in the alert body, broadly speaking? I might be more agreeable to something like that.

For example:

.usa-alert__body > :last-child { margin-bottom: 0; }

This comment suggests the full design system might not be imported so that might be necessary (unless it's an outdated comment).

.usa-alert__body > :last-child { @include u-margin-bottom(0); }

We do. Let me play around and see if one can work. I think I'd have to add that class into the translation files though, and I'm not sure I want to do make that content even messier.

This comment suggests the full design system might not be imported so that might be necessary (unless it's an outdated comment).

That stylesheet is only used for email messages.

svalexander · 2023-02-15T15:40:59Z

app/views/idv/in_person/state_id.html.erb

  <%= render PageHeadingComponent.new.with_content(t('in_person_proofing.headings.state_id')) %>
 <% end %>

+<%= render AlertComponent.new(


question for my understanding - are we able to know if a user's id is expired or is this an alert that everyone sees when they arrive on this page?

I believe everyone sees it

@svalexander Great question! This alert is static. It's an attempt to help users understand what kinds of IDs are acceptable, so it should display regardless of the status of state id.

allthesignals

Just some minor questions

allthesignals · 2023-02-15T15:47:17Z

app/assets/stylesheets/components/_alert.scss

+
+.usa-alert__body {
+  ul {
+    margin-bottom: 0px;


I don't think I understand the rails component system enough but is this going to affect other usages of usa-alert__body elsewhere?

JackRyan1989 · 2023-02-15T16:57:06Z

Just some minor questions

So it will affect any usage of an unordered list element inside of an element with the `.usa-alert__body' class. Which I think is reasonably specific, but maybe not. Should I ping some other folks @allthesignals ?

JackRyan1989 · 2023-02-15T17:39:26Z

@allthesignals I opted to remove the change to the stylesheet to prevent changes elsewhere in the site. This did end up putting more info into the HTML in the translation files though.

allthesignals · 2023-02-15T17:48:32Z

@allthesignals I opted to remove the change to the stylesheet to prevent changes elsewhere in the site. This did end up putting more info into the HTML in the translation files though.

Cool, I think that's fine. I think it's probably a "less surprising" approach: we use USWDS styling, that margin class is a well-documented thing, and no one will find this adhoc __body class used inline in translation files... did you visually confirm it worked? Wasn't sure based on that comment I linked above.

Thanks!!

allthesignals · 2023-02-15T17:55:17Z

Also, (sorry to belabor it) another approach might be to change AlertComponent to accept a block...

<% AlertComponent.new(**stuff).with_content(t('in_person_proofing.body.state_id.alert_message')) do %>
  <ul>
    <% t('in_person_proofing.body.state_id.alert.forms_of_id').each do |type_of_id| %>
      <li>
        <%= type_of_id %>
      </li>
    <% end %>
  </ul>
<% end %>

And forms_of_id could just be a list!

JackRyan1989 · 2023-02-15T19:54:28Z

Also, (sorry to belabor it) another approach might be to change AlertComponent to accept a block...
<% AlertComponent.new(**stuff).with_content(t('in_person_proofing.body.state_id.alert_message')) do %>
  <ul>
    <% t('in_person_proofing.body.state_id.alert.forms_of_id').each do |type_of_id| %>
      <li>
        <%= type_of_id %>
      </li>
    <% end %>
  </ul>
<% end %>
And forms_of_id could just be a list!

Beautious. This is better than including HTML in the translation files IMO. Thanks!

…component

NavaTim · 2023-02-15T20:13:31Z

spec/features/idv/steps/in_person/verify_step_spec.rb

    click_button t('idv.buttons.change_state_id_label')
    expect(page).to have_content(t('in_person_proofing.headings.update_state_id'))
    fill_in t('in_person_proofing.form.state_id.first_name'), with: 'bad first name'
+    binding.pry


Remove this line.

Suggested change

binding.pry

carmenrosalop · 2023-02-16T16:05:11Z

config/locales/in_person_proofing/fr.yml

          de sécurité sociale.
      state_id:
+        alert_message: Votre carte d’identité délivrée par l’État ne doit pas être
+          périmée. Les pièces d’identité acceptées sont


We might need to add a colon here after "sont" @JackRyan1989

carmenrosalop · 2023-02-16T16:11:05Z

config/locales/in_person_proofing/en.yml

        verify_step_enter_pii: Enter your name, date of birth, state-issued ID number,
          address and Social Security number.
      state_id:
+        alert_message: 'Your state-issued ID must not be expired. Accepted forms of ID are:'


The alert shows below the hint text on the Figma file, but above the hint text on the content file. I would ask Allison which one is the correct placement @JackRyan1989

Looking good!

carmenrosalop

Thanks for the updates!

carmenrosalop · 2023-02-16T17:41:06Z

config/locales/in_person_proofing/en.yml

        verify_step_enter_pii: Enter your name, date of birth, state-issued ID number,
          address and Social Security number.
      state_id:
+        alert_message: 'Your state-issued ID must not be expired. Accepted forms of ID are:'


Looking good!

jack-ryan-nava-pbc added 4 commits February 14, 2023 16:32

Added alert component to state id step page and included relevant tra…

296e3d5

…nslations.

Using with_content to match other render function styles.

6ff3b77

Added small styling chage for list items in alerts

120eedc

changelog: User-facing improvements, In-person proofing, add alert sp…

39f11dd

…ecifying accepted forms of identification

JackRyan1989 requested review from a team, carmenrosalop and jess-fortier and removed request for a team February 15, 2023 15:24

allthesignals reviewed Feb 15, 2023

View reviewed changes

svalexander reviewed Feb 15, 2023

View reviewed changes

allthesignals approved these changes Feb 15, 2023

View reviewed changes

Reverting change in stylesheet. Adding class to translation files.

1049f2b

Ramblings

584dc36

Removed HTML from translation file instead passing in block to alert …

3231185

…component

NavaTim reviewed Feb 15, 2023

View reviewed changes

jack-ryan-nava-pbc added 2 commits February 15, 2023 15:20

Remove binding.pry

14c28fe

Normalize yml

85fa970

carmenrosalop reviewed Feb 16, 2023

View reviewed changes

Adding colon and fixing order of text

bc740e6

JackRyan1989 requested a review from carmenrosalop February 16, 2023 16:37

carmenrosalop approved these changes Feb 16, 2023

View reviewed changes

JackRyan1989 merged commit 90ac7bb into main Feb 16, 2023

JackRyan1989 deleted the jryan/lg-8543-id-number-alert branch February 16, 2023 18:59

solipet mentioned this pull request Feb 21, 2023

Deploy RC 255 to Prod #7858

Merged

Conversation

JackRyan1989 commented Feb 15, 2023

🎫 Ticket

🛠 Summary of changes

📜 Testing Plan

👀 Screenshots

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 Feb 15, 2023 • 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!

aduth Feb 15, 2023 • 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!

allthesignals left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackRyan1989 commented Feb 15, 2023

Uh oh!

JackRyan1989 commented Feb 15, 2023

Uh oh!

allthesignals commented Feb 15, 2023

Uh oh!

allthesignals commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JackRyan1989 commented Feb 15, 2023

Uh oh!

NavaTim Feb 15, 2023 • 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!

carmenrosalop left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

aduth Feb 15, 2023 •

edited

Loading

aduth Feb 15, 2023 •

edited

Loading

allthesignals commented Feb 15, 2023 •

edited

Loading

NavaTim Feb 15, 2023 •

edited

Loading