-
Notifications
You must be signed in to change notification settings - Fork 132
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
LG-14748 Add in person warning to password reset email #11547
Conversation
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.
These are legacy styles for older versions of alert in the design system. Can we use the updated styles instead (see warning.png
and existing usage for IPP outage alerts.
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.
Same here. I will check with the designers.
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.
Update: Designers agree that we should be going with the updated design. I will update this today!
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 updated to the latest design
@@ -91,6 +91,18 @@ h6 { | |||
} | |||
} | |||
|
|||
.usa-alert-no-border { |
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.
Similarly, I think we should be using standard alert appearance and not adding this variation.
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 agree, I was just following what design wanted. I will check-in with the designers to see if using the existing designs for the alert will be okay. 👍🏻
e3c87c0
to
ce0cce0
Compare
cb13119
to
6629d31
Compare
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.
This LGTM! The spacing for the mailer alert component seems to be a bit different from the design system spec, so Shane and I have discussed and plan on handling this in a separate ticket.
Testing Notes: ✅ Scenario: When the user has a pending in-person enrollment ✅ Scenario: (Regression Test) When the user has a pending gpo enrollment ✅ Scenario: (Regression Test) When user has proofed via remote ✅ Scenario: (Regression Test) When user started IPP (picked a PO) - has an establishing enrollment I did not take a screenshot but warning banner does not show |
<tbody> | ||
<tr> | ||
<td style="width:16px;"> | ||
<%= image_tag('email/warning.png', width: 16, height: 14, alt: '', style: 'margin-top: 5px;') %> |
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 know there are many instances where alt text is blank but I still prefer we don't leave it blank. When I test with VoiceOver, it recognizes the image but our users don't have any context or knowledge about it. I think if we have an image, we should have alt text for our users and to be 508 compliant, see GSA resource 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.
done
spec/i18n_spec.rb
Outdated
@@ -74,6 +74,7 @@ class BaseTask | |||
{ key: 'time.formats.event_timestamp', locales: %i[zh] }, | |||
{ key: 'time.formats.full_date', locales: %i[es] }, # format is the same in Spanish and English | |||
{ key: 'time.formats.sms_date' }, # for us date format | |||
{ key: 'user_mailer.reset_password_instructions.in_person_warning_description_html', locales: %i[es fr zh] }, |
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.
Nit: Only if you are updating the code again- a note that this will temporary until translations come in would be nice
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.
Nice work Shane. I added a suggestions. Nothing is blocking.
I was testing more with VoiceOver. I notice the banner does not get read unless you know it is there and click on it. Page reads h1 first. I moved the banner below the h1 but it is still not getting read. Also, I put your content in the table- no luck. It still skips to the h1. I tested the GPO reset PW email. The information about PW reset is read without clicking on it. I am wondering if we should modify our design to be like GPO? Maybe we merge as is and have design rework the mocks a bit? |
6629d31
to
6c8b375
Compare
Nice find Gina! I think this would also apply to other emails that have this type of banner... so we might need an entire re-work for ID-IPP/EIPP emails. |
changelog: User-facing Improvements, In-person Proofing, Add warning banner to password reset email when the user has an in-progress in-person enrollment
6c8b375
to
a8fc357
Compare
<tbody> | ||
<tr> | ||
<td style="width:16px;"> | ||
<%= image_tag('email/warning.png', width: 16, height: 14, alt: 'warning icon', style: 'margin-top: 5px;') %> |
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.
[non-blocking] do you think it would be better to use an svg here or is the png ok? I just saw the styling and thought maybe it would be a bit easier to work with as an svg.
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.
We have to use pngs for email because svgs are not supported by most email clients.
see #10839 (comment)
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.
For the alt text, do you want to ask if we can get translations order for that? Else you can use
two_factor_authentication.important_alert_icon
with is important alert icon
This is non- blocking
@@ -74,6 +74,7 @@ class BaseTask | |||
{ key: 'time.formats.event_timestamp', locales: %i[zh] }, | |||
{ key: 'time.formats.full_date', locales: %i[es] }, # format is the same in Spanish and English | |||
{ key: 'time.formats.sms_date' }, # for us date format | |||
{ key: 'user_mailer.reset_password_instructions.in_person_warning_description_html', locales: %i[es fr zh] }, # Temporary until spanish, french, and chinese translations come in. |
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 for the comment! 🚀
Thanks for addressing my comments. Looks good to me. |
🎫 Ticket
Link to the relevant ticket:
LG-14748
🛠 Summary of changes
Add a warning banner to the password reset email for users with a pending in-person enrollments.
Scenario: When the user does not have a pending in-person enrollment
Scenario: When the user has a pending in-person enrollment
(Regression Test) Scenario: When the user has a pending gpo enrollment
Note: You might have enable gpo flow from the environment vars
/verify/phone
page👀 Screenshots
Updated Password Reset Email
English:
Spanish:
French:
Chinese: