Skip to content

LG-7620: Update IPP CTA language to include Baltimore option#7029

Closed
allthesignals wants to merge 3 commits into18F:mainfrom
allthesignals:wmg/7620-ipp-cta-updates
Closed

LG-7620: Update IPP CTA language to include Baltimore option#7029
allthesignals wants to merge 3 commits into18F:mainfrom
allthesignals:wmg/7620-ipp-cta-updates

Conversation

@allthesignals
Copy link
Contributor

@allthesignals allthesignals commented Sep 26, 2022

Update FR, ES, and EN IPP CTA language to include Baltimore option.

Ran these changes through make normalize_yaml

Follow up should include updates to: https://github.com/18F/identity-site/blob/20e3e81cd7ff94491f8709e4b1d2b52efd18bfa8/content/_help/verify-your-identity/verify-your-identity-in-person._en.md.

🎫 Ticket

Link to the relevant ticket.

🛠 Summary of changes

Change IPP CTA language to include Baltimore, MD. Note the intentional newline after "or".

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Check that CI passes
  • Visual check after local setup?

👀 Screenshots

If relevant, include a screenshot or screen capture of the changes.

Before: image
After: image

@allthesignals allthesignals marked this pull request as ready for review September 26, 2022 20:52
Includes updates to FR, ES, and EN locales.
@allthesignals allthesignals force-pushed the wmg/7620-ipp-cta-updates branch from d8a4066 to 64da62e Compare September 26, 2022 21:37
.troubleshooting-options__heading {
@include u-margin-y(1.5);
@extend %h3;
white-space: pre;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there special guidance on adding new CSS rules?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bit of general guidance in this documentation: https://github.com/18F/identity-idp/blob/main/docs/frontend.md#css--html

For this, I would be most cautious about potential impact on other places we're using this troubleshooting component, whether white-space: pre could have any undesirable side-effects like preventing natural line breaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure! There seem to be some other CSS options for white-space that might help avoid that scenario, but I'm just going to investigate the solution you propose. Thank you!

.troubleshooting-options__heading {
@include u-margin-y(1.5);
@extend %h3;
white-space: pre;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a bit of general guidance in this documentation: https://github.com/18F/identity-idp/blob/main/docs/frontend.md#css--html

For this, I would be most cautious about potential impact on other places we're using this troubleshooting component, whether white-space: pre could have any undesirable side-effects like preventing natural line breaks.

Comment on lines +189 to +191
are_you_near: 'Are you located near Washington, D.C. or

Baltimore, MD?'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As an alternative to styling this with white-space: pre, I wonder if we could use HTML, such as the <wbr> element?

Suggested change
are_you_near: 'Are you located near Washington, D.C. or
Baltimore, MD?'
are_you_near_html: 'Are you located near Washington, D.C. or<wbr>Baltimore, MD?'

Because the string is used in JavaScript (React), there's a bit of extra work to make the HTML show up. Specifically, there is a helper function formatHTML in the @18f/identity-react-i18n package which allows for HTML to be inserted. Unfortunately, I just noticed that (a) it's not very well-documented outside this function DocBlock, and (b) apparently it doesn't work for self-closing (void) tags like <wbr />. I can try to see if I can get that fixed up, since I was the one who had implemented the helper.

Copy link
Contributor

@aduth aduth Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can try to see if I can get that fixed up, since I was the one who had implemented the helper.

Fix proposed at #7034

Copy link
Contributor Author

@allthesignals allthesignals Sep 27, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, @aduth — I went ahead and pulled in your code (temporarily) to develop on top of this. This is what I'm trying:

          heading={formatHTML('Are you located near Washington, D.C.<wbr/>or Baltimore, MD?', {
            wbr: () => <wbr />,
          })}

And seeing:

image

I removed the i18n lookup for simplicity and testing. I've also tried { wbr: 'wbr' }, as it's showing here. Hmm I'll keep poking around. Thanks!

This was a yarn workspace issue... update:

Now, I'm seeing it render a proper node, however, it drops the rest of the text. Going to play around some more...:

image

@allthesignals allthesignals changed the title Update IPP CTA language to include Baltimore option LG-7620: Update IPP CTA language to include Baltimore option Sep 27, 2022
@svalexander
Copy link
Contributor

in the before section under screenshots can you add one of what that screen looks like w/o these changes?

@allthesignals
Copy link
Contributor Author

Pushed a commit that anticipates merge of #7034

Comment on lines 85 to 87
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It appears that the heading prop uses string as the TypeScript type, which isn't compatible with the return value of formatHTML (ReactNode). I think it would be straightforward/valid to update the string type to ReactNode to indicate that the prop can be anything that React can render.

Updating the type on this line from string to ReactNode (already imported in the file):

Original error:

$ yarn typecheck
yarn run v1.22.19
$ tsc
app/javascript/packages/document-capture/components/document-capture-troubleshooting-options.tsx:85:11 - error TS2322: Type 'ReactNode' is not assignable to type 'string | undefined'.
  Type 'null' is not assignable to type 'string | undefined'.

85           heading={formatHTML(t('idv.troubleshooting.headings.are_you_near'), {
             ~~~~~~~

  app/javascript/packages/components/troubleshooting-options.tsx:17:3
    17   heading?: string;
         ~~~~~~~
    The expected type comes from property 'heading' which is declared here on type 'IntrinsicAttributes & TroubleshootingOptionsProps'


Found 1 error in app/javascript/packages/document-capture/components/document-capture-troubleshooting-options.tsx:85

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Copy link
Contributor

@aduth aduth Sep 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple observations:

  • The <wbr /> doesn't appear to create any space on its own in situations where there might not need to be a line break, so I think we should include one:
Suggested change
are_you_near: 'Are you located near Washington, D.C.<wbr/>or Baltimore, MD?'
are_you_near: 'Are you located near Washington, D.C. <wbr/>or Baltimore, MD?'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aduth Hmm, I see what you mean with <wbr />. I don't think it does exactly what we need. I think when I tried it in the app, it seemed to accidentally work correctly.

My understanding is that design wants to force a line break there in every situation, rather than simply to prefer it there. Let me know if that sounds wrong to you, but I'm going to switch to <br/>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. My understanding is that it prevents the try_again portion of the message from wrapping to a new line. Maybe this was solving a design issue? I can't infer much of the reasoning behind it from the blame. That approach also does similar string manipulation. I wonder if the helper belongs alongside formatHtml...

Still, we use lots of HTML throughout the locales yaml files. In the spirit of using a single approach, maybe that prior attempt should be changed to use explicit &nbsp; inside the yaml content? And remove the post-processing helper? Although that might miss something that the usage of the unicode representation of the non-breaking space was trying to solve...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is interesting. My understanding is that it prevents the try_again portion of the message from wrapping to a new line. Maybe this was solving a design issue? I can't infer much of the reasoning behind it from the blame. That approach also does similar string manipulation. I wonder if the helper belongs alongside formatHtml...

Still, we use lots of HTML throughout the locales yaml files. In the spirit of using a single approach, maybe that prior attempt should be changed to use explicit &nbsp; inside the yaml content? And remove the post-processing helper? Although that might miss something that the usage of the unicode representation of the non-breaking space was trying to solve...

Yes, as the person who had implemented that, I can confirm it was for design / legibility reasons to explicitly control the line breaks.

I don't have a strong opinion whether that should happen in the YAML or as a post-processing step, but I have a soft preference for the latter, to keep the YAML concerned with just the content, and any presentational concerns in the realm of the application code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should continue this in the next PR, but I'm wondering if the best practice would be: prefer to separate content into segments and keep HTML/display logic in application code. In this case, maybe we'd maintain a list of IPP pilot sites, which will change as we scale up. Not try to belabor this, but I'll try an alternative approach... (still need to get Gitlab access)

@allthesignals allthesignals force-pushed the wmg/7620-ipp-cta-updates branch from 1bfa264 to f922b0f Compare September 28, 2022 18:23
@allthesignals allthesignals requested a review from aduth September 28, 2022 19:59
@allthesignals
Copy link
Contributor Author

Closing in favor of #7058.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants