LG-14641 | Update "letter on the way" screen by n1zyy · Pull Request #11346 · 18F/identity-idp

n1zyy · 2024-10-15T17:56:50Z

🎫 Ticket

Link to the relevant ticket:
LG-14641

🛠 Summary of changes

UI improvements, including translations.

changelog: User-Facing Improvements, GPO flow, UI tweaks

n1zyy · 2024-10-15T18:04:02Z

app/controllers/redirect/marketing_site_controller.rb

+      redirect_to_and_log(MarketingSite.base_url)
+    end
+  end
+end


Part of the story here was that we want to redirect to straight "www.login.gov" if the user clicks "Exit Login.gov", and log an event. The most pragmatic way of doing this I saw was to just add a new controller, rather than weirdly cramming it into the HelpCenterController.

Makes sense!

n1zyy · 2024-10-15T18:05:20Z

app/presenters/idv/by_mail/letter_enqueued_presenter.rb

-          t('idv.buttons.continue_plain')
-        end
+        t('idv.cancel.actions.exit', app_name: APP_NAME)
      end


(Context: We settled on the button always having the same text, but keeping the destination dynamic.)

n1zyy · 2024-10-15T18:06:08Z

app/presenters/idv/by_mail/letter_enqueued_presenter.rb

          return_to_sp_cancel_path(step: :verify_address, location: :come_back_later)
        else
-          account_path
+          marketing_site_redirect_url


Taking the user to their account page when they clicked "Exit Login.gov" was very random.

(Arguably, so is taking them to the static site homepage, but it's less-inexplicable.)

FWIW we have a big epic on the roadmap that is all about removing these if sp then x else y type constructions--in reality, every IdV attempt should be associated with an SP.

n1zyy · 2024-10-15T18:08:39Z

app/views/idv/by_mail/letter_enqueued/show.html.erb

+  <p class="margin-top-2">
+    <%= t('idv.messages.come_back_later_no_sp_html') %>
+  </p>
+<% end %>


And, context here: if there is no SP, we don't want to show the second bullet point telling them to contact nil, so we omit it. But a one-item bulleted list is silly, so we go for a paragraph tag instead.

This is a good catch! I will update #11351 to do the same.

app/views/idv/by_mail/letter_enqueued/show.html.erb

n1zyy · 2024-10-15T18:09:14Z

app/views/idv/by_mail/letter_enqueued/show.html.erb

      @presenter.button_text,
      @presenter.button_destination,
-      class: 'usa-button usa-button--big usa-button--wide',
+      class: 'usa-button usa-button--big usa-button--wide margin-top-2',


I am not a designer, but the button was otherwise awkwardly close to the text.

app/presenters/idv/by_mail/letter_enqueued_presenter.rb

jmhooper · 2024-10-15T20:23:42Z

app/views/idv/by_mail/letter_enqueued/show.html.erb

-<p class="padding-top-3 padding-bottom-3">
-  <%= t('idv.messages.come_back_later_html') %>
-</p>
+<% if @presenter.has_sp? %>


I was working on a companion to this story with @lmgeorge. We came up with a @presenter.show_contact_sp_instructions? method which made things a little more self-documenting.

We also did not add a list to the YAML file. We had a descriptor for each one.

When we see that PR up we should work on squaring those up since they are essentially the same thing.

PR in question: #11351, and more specifically in the ERB template.

Thanks! I've matched this naming.

I had initially assumed these were using the same presenter, but they are not. I think I'm OK duplicating the pattern / 1-liner method rather than trying to extract stuff like this:

def sp_name sp&.friendly_name end

config/locales/en.yml

n1zyy · 2024-10-15T22:09:31Z

config/locales/en.yml

-idv.messages.come_back_later_html: 'Letters take <strong>5 to 10 days</strong> to arrive. Sign back in to enter your verification code once you get your letter.'
+idv.messages.come_back_later: Sign back in and enter the verification code when your letter arrives.
 idv.messages.confirm: We secured your verified information
+idv.messages.contact_sp_html: Contact&nbsp;%{sp_link} if you need to access their services before your letter arrives.


OK, so: the i18n linter yells at me because contact_sp_html is an _html string but doesn't contain any HTML. %{sp_link} is a hyperlink we interpolate. Taking the _html out causes raw HTML to be displayed.

I have put an otherwise-needless   here to make things happy, but this is obviously a very gross approach. I am out tomorrow (Wednesday) if anyone wants to apply an obvious correction; otherwise, I'll pick this up Thursday morning.

I think you can suffix the interpolated key as %{sp_link_html} for it to understand the HTML is in the interpolated value.

identity-idp/spec/i18n_spec.rb

Lines 387 to 391 in b77c611

def contains_html?(value)

Array(value).flatten.compact.any? do |str|

html_tags?(str) || html_entities?(str) || likely_html_interpolation?(str)

end

end

Ah-ha! Thanks for this pointer.

Since it still took me a little bit of trial-and-error, a pointer for the next person:

The translation key should still keep the _html suffix

Including an interpolation ending in _html causes the linter to not object to the translation not directly including HTML

n1zyy · 2024-10-15T22:10:31Z

spec/views/idv/by_mail/letter_enqueued/show.html.erb_spec.rb


 RSpec.describe 'idv/by_mail/letter_enqueued/show.html.erb' do
-  let(:service_provider) { '🔒🌐💻' }
+  let(:service_provider) { create(:service_provider) }


AFAICT we were always expecting an instance of a ServiceProvider in the presenter, not a (emoji-filled) string, so I'm not sure how this ever passed.

Yeah that's not ideal. It looks like this service_provider gets passed into the Idv::Session constructor which then gets read by the presenter in a basic if sp type thing. So any truthy value would pass here

Some day I will learn to always run this when working with translations. But today was not that day.

matthinz · 2024-10-22T23:28:14Z

spec/views/idv/by_mail/letter_enqueued/show.html.erb_spec.rb

@@ -53,8 +53,8 @@

    it 'renders a return to account button' do


maybe update this it here since it's no longer a "return to account" button

LG-14651 | Update "letter on the way" screen

fa5a2f7

changelog: User-Facing Improvements, GPO flow, UI tweaks

n1zyy requested a review from a team October 15, 2024 18:02

n1zyy commented Oct 15, 2024

View reviewed changes

n1zyy added 2 commits October 15, 2024 15:49

lintfixes

381de65

Merge branch 'main' into mattw/LG-14651_letter_on_the_way

391fc66

jmhooper reviewed Oct 15, 2024

View reviewed changes

app/presenters/idv/by_mail/letter_enqueued_presenter.rb Outdated Show resolved Hide resolved

jmhooper reviewed Oct 15, 2024

View reviewed changes

config/locales/en.yml Outdated Show resolved Hide resolved

Fix tests and lint issues

ed0871e

n1zyy commented Oct 15, 2024

View reviewed changes

n1zyy added 5 commits October 17, 2024 13:33

Merge branch 'main' into mattw/LG-14651_letter_on_the_way

02f62e4

Merge branch 'main' into mattw/LG-14651_letter_on_the_way

464ef5f

Addresses PR feedback; harmonizes with LMG's PR for the email template

76d1c57

normalize_yaml

deb1864

Some day I will learn to always run this when working with translations. But today was not that day.

Merge branch 'main' into mattw/LG-14651_letter_on_the_way

17e3c94

matthinz reviewed Oct 22, 2024

View reviewed changes

matthinz approved these changes Oct 22, 2024

View reviewed changes

n1zyy changed the title ~~LG-14651 | Update "letter on the way" screen~~ LG-14641 | Update "letter on the way" screen Oct 23, 2024

n1zyy added 2 commits October 25, 2024 12:29

Merge branch 'main' into mattw/LG-14651_letter_on_the_way

7add40f

Fix incorrect example

a813f31

n1zyy merged commit 0e2e6c5 into main Oct 25, 2024

n1zyy deleted the mattw/LG-14651_letter_on_the_way branch October 25, 2024 16:45

solipet mentioned this pull request Oct 29, 2024

Deploy RC 426 to Production #11416

Merged

	def contains_html?(value)
	Array(value).flatten.compact.any? do \|str\|
	html_tags?(str) \|\| html_entities?(str) \|\| likely_html_interpolation?(str)
	end
	end

Conversation

n1zyy commented Oct 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎫 Ticket

🛠 Summary of changes

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

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!

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!

n1zyy Oct 15, 2024 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

n1zyy commented Oct 15, 2024 •

edited

Loading

n1zyy Oct 15, 2024 •

edited

Loading