Skip to content
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

Internationalized NoRent primary pages #1405

Merged
merged 17 commits into from
May 14, 2020
Merged

Internationalized NoRent primary pages #1405

merged 17 commits into from
May 14, 2020

Conversation

sraby
Copy link
Member

@sraby sraby commented May 7, 2020

This PR internationalizes the homepage, "The Letter" page, about page, faqs page, and log out page on the NoRent site. It also internationalizes any component referenced in the primary pages (like our social share icons) as well as the header and footer.

@sraby sraby changed the title I18n primary pages Internationalized NoRent primary pages May 7, 2020
@sraby sraby requested a review from toolness May 7, 2020 20:28
Copy link
Collaborator

@toolness toolness left a comment

Choose a reason for hiding this comment

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

Noooice this looks good! Just a few comments, some of them somewhat meandering/blathery. I'm not sure how translators will react to this so a lot of it is speculative too, feel free to disagree!

It might be helpful to also run yarn lingui:extract and just look at the resulting messages.po files--but not commit them--to see what the translators will see? As I mention in one of the comments it might be good not to have the message IDs be too massive but I'm not sure.

frontend/lib/norent/about.tsx Outdated Show resolved Hide resolved
frontend/lib/norent/components/footer.tsx Show resolved Hide resolved
Comment on lines +95 to +98
<Trans>
<NorentLogo size="is-64x64" color="white" />{" "}
<span>brought to you by JustFix.nyc</span>
</Trans>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if it might make sense to add a comment="This is the NoRent logo followed by the text 'brought to you by justfix.nyc'" attribute or something here, just so translators know that the first tag (which will show up as like, <0> or something nonsensical) has meaning to the translation... hmmm.

Copy link
Member Author

Choose a reason for hiding this comment

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

This will be resolved once I implement your fix from below! https://github.com/JustFixNYC/tenants2/pull/1405/files#r421808840

frontend/lib/norent/components/helmet.tsx Show resolved Hide resolved
Comment on lines +19 to +25
<Trans>
<NorentLogo size="is-96x96" color="dark" />{" "}
<span className="subtitle">
letters sent by tenants across the USA
</span>
<p className="is-uppercase">Since April 2020</p>
</Trans>
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's probably obvious to translators, but it might mayyybe be useful to add a comment attribute to the <Trans> explaining that a number appears before the translated text? I dunno just a thought.

frontend/lib/norent/data/faqs-content.tsx Outdated Show resolved Hide resolved
frontend/lib/norent/homepage.tsx Outdated Show resolved Hide resolved
Comment on lines +84 to +86
<Trans>
Here’s what you can do with <NorentLogo size="is-128x128" />
</Trans>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, I wonder if a comment attribute would help the translator make better sense of this? Or on the other hand, Crowdin does make the file and line number visible to the translator, but it doesn't seem to have any way of providing a hyperlink directly to the location on GitHub... it would be cool if it did, at least then the translator could click on it to see the <NorentLogo>.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OH WAIT! You know what might be awesome?!! If this JSX actually read like this:

<Trans>
  Here's what you can do with <NorentLogo size="is-128x128">NoRent</NorentLogo>
</Trans>

The <NorentLogo> could simply not render its children, but instead use the children as its alt text. And this would show up to translators as something like:

Here's what you can do with <0>NoRent</0>

That would be a lot easier for them to understand than the current code, which I think would read to translators as:

Here's what you can do with <0>

it would also allow them to easily change the alt text I guess, although I'm not sure if that's desirable or not.

Anyways just an idea.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Err if you actually think this idea is good, I would hold on actually implementing it for another PR though...

Copy link
Member Author

@sraby sraby May 8, 2020

Choose a reason for hiding this comment

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

That's a really good idea!! Ok I'll save this for another PR to work on next week.

notice, the COVID-19 emergency may impact my ability to
pay rent.
</p>
<br />
Copy link
Collaborator

Choose a reason for hiding this comment

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

These <br/>s inside <Trans> content concern me a bit because they're essentially formatting/styling rather than content, and they could confuse translators (especially since they can't see the actual type of tag and will just see e.g. <1>... and if we ever remove them, it will cause translators to have to re-translate the text, which could be frustrating. It'd be nice to get rid of them but I also don't want this PR to have to also tackle CSS stuff, so maybe file an issue to address it later?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hey hey, any thoughts on this? I think it's the only part of my review that you haven't addressed but I might have missed something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oooh sorry! I filed an issue for this as you suggested (#1424) but forgot to circle back and link it here.

frontend/lib/norent/homepage.tsx Show resolved Hide resolved
@sraby
Copy link
Member Author

sraby commented May 8, 2020

@toolness, thanks for your thoughtful review! I ran into some problems adding in comment props to our <Trans> tags as well as using the defineMessage macro. Both attempts rendered an error saying these properties/exports don't exist in the module. I thought it would be good to take a pause and circle back to this on Monday.

@sraby
Copy link
Member Author

sraby commented May 12, 2020

@toolness as I suspected, Tahnee does have a specific framing and system for when we use Title case vs. Sentence case. I summed up as best I could here:

When we use Sentence case:

  • Default for page/section headers (I.e. "Cant pay rent?" on the landing page)
  • Text in a list (I.e. "Build a letter using our free letter builder" under the "Here's what you can do with norent" section on the landing page, and "Going on rent strike" under "Locally supported")

When we use Title case:

  • Text that acts as a label, or a caption for an image/icon (I.e. "Legal Protections" under "How it works" on the landing page.
  • Any phrase that corresponds to a specific, formal entity rather than a general thing (I.e "Frequently Asked Questions" not "Frequently asked questions" and "Our Partners" not "Our partners".

Given that, it seems that the only error in the content so far was the section titles on the About page. I changed that so we should be good to go.

Let me know if you have any qualms about this and we can try and set up a time to talk through it together.

@toolness
Copy link
Collaborator

@toolness, thanks for your thoughtful review! I ran into some problems adding in comment props to our <Trans> tags as well as using the defineMessage macro. Both attempts rendered an error saying these properties/exports don't exist in the module. I thought it would be good to take a pause and circle back to this on Monday.

Ok, I just dug into this and discovered that the docs on Lingui's website actually appear to be for the bleeding-edge, in-development version of Lingui, not the latest official release, and defineMessage is one of the new bleeding-edge features! I just filed lingui/js-lingui#692 so hopefully folks don't get confused by this like we just did.

I guess in the meantime there's really nothing we can do to improve these messages, except maybe splitting them up into separate catalogs for localizers, but that might be a bleeding-edge new feature too for all I know, so let's just shelve the idea for now.

Thanks for the explanation on sentence case vs. title case--we definitely need to make a content guide at some point that spells all this out! I guess that's one for #323, which really ought to be in Clubhouse at this point...

@sraby
Copy link
Member Author

sraby commented May 14, 2020

Ahhh damn, I wanna be on the "bleeding-edge" too! Makes sense though as to why we cant use those features yet... ok well in that case, this should be good to go for merging right? With the exception of #1405 (comment) which I'll do in a separate PR after we merge this one into master.

@toolness toolness merged commit 3074e1c into master May 14, 2020
@toolness toolness deleted the i18n-primary-pages branch May 14, 2020 20:05
@toolness toolness mentioned this pull request May 14, 2020
33 tasks
sraby added a commit that referenced this pull request May 15, 2020
This PR implements @toolness's suggesstion from #1405 (comment). It refactors our <NorentLogo> component to take its child and use it as the alt text for the image. Now, translators on CrowdIn can also see the alt text, which will help give context when they translate sections that include the NoRent logo inline.
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.

2 participants