LG-6343: Implement email template for "Ready to verify"#6585
Conversation
f308b5f to
d488d81
Compare
4b3b578 to
87fa839
Compare
59b74d6 to
ac07bc6
Compare
87fa839 to
536dc49
Compare
This came through in pretty good shape to both Gmail and Apple Mail. The one stand-out was a missing icon for the alert banner in Gmail, since apparently SVG images are not well-supported. This is probably not critical, though I could just create a raster version of the icon to use in the email template. |
Better email support See included README.md See: #6585 (comment)
|
Marking this as ready to review, though for design review consideration I want to acknowledge that there are some limitations of overall email support and our email templates which means that the template may not be a 1-to-1 match with the approved designs. Some of these can be fine-tuned (e.g. margins and paddings), and some cannot (no custom font support in emails). A lot of this is also inherited from the existing default email styling, so it could be worth considering consistency or a more holistic approach to certain adjustments (e.g. paragraph spacing or heading stylings). Let me know what kinds of improvements we'd be most interested in making (if any), and I'll try to judge how best to approach them. |
|
Also, the preview from the screenshot is just that: a preview. Email clients have notoriously inconsistent rendering for email messages. I have this set up in my sandbox environment to trigger a real email message and I can forward that along to anyone if it's helpful. |
kellular
left a comment
There was a problem hiding this comment.
Given the styling limitations that Andrew mentioned, I think the email looks okay and even better if we can address the two comments I added.
| <%= t('user_mailer.in_person_ready_to_verify.intro') %> | ||
| </p> | ||
|
|
||
| <table class="info-alert margin-y-4"> |
There was a problem hiding this comment.
Is it possible to make the alert heading:
- the same font size as the alert body copy
- bolded
I think this is one way to introduce some consistency in style between the email and the Login page.
There was a problem hiding this comment.
The font size difference is present in the Figma designs (18px for bolded heading, 16px for unbolded text). It'd be easy enough for me to standardize them as the same body font size though if that's what we want.
There was a problem hiding this comment.
I can see how it's a bit more noticeable a difference to control these font sizes in the email template though, since the default body font size is 13px in the emails (vs. 16px elsewhere in the app).
There was a problem hiding this comment.
I can see how it's a bit more noticeable a difference to control these font sizes in the email template though, since the default body font size is 13px in the emails (vs. 16px elsewhere in the app).
This is a good point. Might be worth the design team looking at the email styling after pilot.
| <%= @presenter.selected_location_details['state'] %> | ||
| <%= @presenter.selected_location_details['zip5'] %>-<%= @presenter.selected_location_details['zip4'] %> | ||
| </div> | ||
| <h3 class="font-sans-sm margin-y-0 text-normal text-bold"><%= t('in_person_proofing.body.barcode.retail_hours') %></h3> |
There was a problem hiding this comment.
Could we update "Retail hours" so that:
- the font size is the same as our body copy
- bolded
There was a problem hiding this comment.
Could we update "Retail hours" so that:
- the font size is the same as our body copy
- bolded
@kellular Similar to my previous comment, this is also how it's designed in the Figma document. I can also change this one to use standard body font size.
changelog: Upcoming Features, In-Person Proofing, Implement "Ready to verify" email notification
Should have same semantics in supported browsers Resources: - https://www.caniemail.com/features/html-semantics/ - https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/figure_role See: https://github.com/18F/identity-idp/pull/6585/files#r922457944
Semantics would be nice, but won't want to risk content being hidden due to lack of support
Better email support See included README.md See: #6585 (comment)
f960468 to
52b2795
Compare
|
@kellular I updated the font sizes in 52b2795 to standardize the alert and "Retail hours" headings with surrounding body copy. Note that I applied this to both the email template, as well as the "Ready to verify" page (since the inconsistency was there as well). Can you take another look and let me know if this looks alright now?
|
tomas-nava
left a comment
There was a problem hiding this comment.
approve!
But I've made a bunch of suggestions that will make the work merge more cleanly with #6624
| 'name' => 'BALTIMORE — Post Office™', | ||
| 'streetAddress' => '900 E FAYETTE ST RM 118', | ||
| 'city' => 'BALTIMORE', | ||
| 'state' => 'MD', | ||
| 'zip5' => '21233', | ||
| 'zip4' => '9715', | ||
| 'phone' => '555-123-6409', | ||
| 'hours' => [ | ||
| { | ||
| 'weekdayHours' => '8:30 AM - 4:30 PM', | ||
| }, | ||
| { | ||
| 'saturdayHours' => '9:00 AM - 12:00 PM', | ||
| }, | ||
| { | ||
| 'sundayHours' => 'Closed', | ||
| }, | ||
| ], |
There was a problem hiding this comment.
| 'name' => 'BALTIMORE — Post Office™', | |
| 'streetAddress' => '900 E FAYETTE ST RM 118', | |
| 'city' => 'BALTIMORE', | |
| 'state' => 'MD', | |
| 'zip5' => '21233', | |
| 'zip4' => '9715', | |
| 'phone' => '555-123-6409', | |
| 'hours' => [ | |
| { | |
| 'weekdayHours' => '8:30 AM - 4:30 PM', | |
| }, | |
| { | |
| 'saturdayHours' => '9:00 AM - 12:00 PM', | |
| }, | |
| { | |
| 'sundayHours' => 'Closed', | |
| }, | |
| ], | |
| 'name' => 'BALTIMORE', | |
| 'streetAddress' => '900 E FAYETTE ST RM 118', | |
| 'addressLine2' => 'BALTIMORE, MD 21233-9715', | |
| 'phone' => '555-123-6409', | |
| 'weekdayHours' => '8:30 AM - 4:30 PM', | |
| 'saturdayHours' => '9:00 AM - 12:00 PM', | |
| 'sundayHours' => 'Closed', |
This is the format you can expect from selected_location_details, as of #6624
| <%= @presenter.selected_location_details['city'] %>, | ||
| <%= @presenter.selected_location_details['state'] %> | ||
| <%= @presenter.selected_location_details['zip5'] %>-<%= @presenter.selected_location_details['zip4'] %> |
There was a problem hiding this comment.
| <%= @presenter.selected_location_details['city'] %>, | |
| <%= @presenter.selected_location_details['state'] %> | |
| <%= @presenter.selected_location_details['zip5'] %>-<%= @presenter.selected_location_details['zip4'] %> | |
| <%= @presenter.selected_location_details['addressLine2'] %> |
With the difference in selected_location_details noted in my last comment. I've made the same change to the page view here in #6624.
| EnrollmentCodeFormatter.format(enrollment_code) | ||
| end | ||
|
|
||
| def selected_location_hours(prefix) |
There was a problem hiding this comment.
The body of this method should change to the below, to accommodate the difference in format of selected_location_details noted in previous comments.
hours = selected_location_details["#{prefix}Hours"]
return localized_hours(hours) if hoursThis is also done here in PR #6624.
There was a problem hiding this comment.
Hm, if I make those changes here before #6624 is merged, then it would break the page rendering for "Ready to verify"? 🤔
I guess I can plug the same changes noted previously into this code:
identity-idp/app/controllers/api/verify/password_confirm_controller.rb
Lines 101 to 120 in 17d7e30
There was a problem hiding this comment.
@tomas-nava Yeah, there's a lot from #6624 that I'd have to pull into this branch to be able to use the updated selected location shape. Could we merge this without those and update #6624? Or, conversely, merge #6624 and I'll update this one?
GitLab crashing again




Why: As a public user trying to proof online I want to see the details of my newly scheduled in-person proofing visit to the USPS as an email so that I can reference it later.
Testing Instructions:
Screenshot: