Skip to content

Conversation

@sraby
Copy link
Member

@sraby sraby commented Aug 25, 2020

This PR internationalizes the entire Letter of Complaint front end. As part of this PR, I've also internationalized our full Onboarding flow that get's used within the LOC flow. See #1652 for details on that work.

TO DO:

  • Figure out how to internationalize the MIN_DAYS_TEXT snippet from the access-dates-validation.json file in the common-data directory. The issue here is that this piece of text gets used in both the frontend (access-dates.tsx) as well as the backend (loc/forms.py).
  • Figure out: do we need to internationalize the LOC sample props and sample page within letter-content.tsx?
  • Internationalize (or figure out a different solution for) the "X characters remaining" text for inputting custom issues
  • While it's not actually part of the front-end, we need to make sure we actually localize the LOC reminder we send to users 24 hours after they've signed up with LOC intent, but before they've sent a letter!

@sraby sraby marked this pull request as draft August 25, 2020 12:53
@sraby sraby changed the title Internationalized the Letter of Complaint front-end Internationalize the Letter of Complaint front-end Aug 25, 2020
@sraby sraby changed the title Internationalize the Letter of Complaint front-end [WIP] Internationalize the Letter of Complaint front-end Aug 25, 2020
@sraby sraby requested a review from toolness September 1, 2020 11:24
@sraby
Copy link
Member Author

sraby commented Sep 1, 2020

@toolness this should be ready for your review! If you can, I'd love it if you could take a look at the tests that are failing— all seem to be manifestations of the same error:

console.error
      Error: Uncaught [ReferenceError: maxRecipients is not defined]

The errors started showing up after I internationalized the "email this XXX to a trusted friend" feature that shows up on the LOC confirmation page, as well as in other places. As far as I know, the only relevant change is that I added Trans tags around this bit of text that includes the maxRecipients variable:

               <Trans>
                 You can use the form below if you'd like us to email your{" "}
                  {noun} to up to {maxRecipients} addresses.
                </Trans>

and maxRecipients is a common data object imported at the top of the file:

import { maxRecipients } from "../../../common-data/email-attachment-validation.json";

I was able to find a potentially related problem logged as an issue for lingui ( lingui/js-lingui#514 ), but it still seemed odd that this was popping up now. I thought at this stage we would've run into this error before?

@toolness
Copy link
Collaborator

toolness commented Sep 1, 2020

Thanks! I'm not sure what's going on there--it's especially weird/annoying that this problem only crops up in Jest and nowhere else--but I ended up using the workaround mentioned in the lingui issue and that seems to fix things.

@toolness
Copy link
Collaborator

toolness commented Sep 1, 2020

Regarding the MIN_DAYS_TEXT internationalization: I think that instead of using that, we should just commit to always specifying the minimum days in terms of either days or weeks, and then use that number in both back-end and front-end internationalized text. So in other words, the messages to localize go from e.g. First access date (at least ${MIN_DAYS_TEXT} from today) to either ``First access date (at least ${MIN_DAYS} days from today)orFirst access date (at least ${MIN_WEEKS} weeks from today)`. What do you think?

@sraby
Copy link
Member Author

sraby commented Sep 1, 2020 via email

@toolness
Copy link
Collaborator

toolness commented Sep 1, 2020

Noice yeah that sounds reasonable to me. I guess we should run it by the content folks just to make sure they are OK with it.

@toolness
Copy link
Collaborator

toolness commented Sep 1, 2020

Ok just got the 👍 from analisa that it's fine to use days across the board!

Comment on lines 44 to 46
{landlord.name || li18n._(t`UNKNOWN LANDLORD`)}
<br />
{landlord.address || "UNKNOWN ADDRESS"}
{landlord.address || li18n._(t`UNKNOWN ADDRESS`)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oops don't worry about internationalizing these--this text is actually never supposed to show up in real-world situations, so we shouldn't burden translators with having to translate it.

@toolness
Copy link
Collaborator

Ok, I parceled out bits of this PR that aren't relevant to the content audit (which is what is blocking us from merging this PR) into #1656 and #1657 and merged those into master, which will give them some time to bake in production, make this PR less massive, and reduce merge conflict headaches down the road.

@sraby
Copy link
Member Author

sraby commented Sep 10, 2020

Oh, nice! Good call @toolness.

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