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

Add foundational internationalization support via lingui #186

Merged
merged 18 commits into from
Nov 26, 2019

Conversation

toolness
Copy link
Contributor

@toolness toolness commented Nov 20, 2019

Note: This was first started in #184, but since I posted a link to that in lingui's GitHub issues and folks might look at it to debug the bug I reported, I decided to build on it in a separate PR to keep the other one nice and simple.

This adds basic internationalization support via the splendid lingui library, which allows us to internationalize without sacrificing code readability or type safety. It will also integrate fine with Crowdin, our preferred localization workflow management solution.

The new functionality has been tested to work properly on IE 11 and the latest versions of Chrome and Firefox.

What users see

Every URL path should now start with /en/ or /es/ (or another locale code), and if it doesn't, we simply redirect to the pathname with a default/auto-detected locale prepended (this makes legacy links work).

A new language switcher is available at the top-right of the page, to switch between our two current locales:

image

What's been internationalized

This PR only actually internationalizes (and crudely translates to Spanish) a few strings, selected because they exercise different parts of lingui's functionality, to make sure everything integrates properly:

  • The title of the site, to make sure the basic <Trans> macro/element works properly in its most trivial use case.
  • The "emergency" text label in the canvas-based indicators visualization, to make sure lingui's JS API works.
  • The first paragraph of the portfolio summary tab, to make sure the <Plural> macro/element works properly and generates ICU MessageFormat strings for localizers.

Feature flagging

Because I'd like to merge this without blocking on actually translating everything, I've added a feature flag, REACT_APP_ENABLE_PUBLIC_FACING_I18N, which controls whether to actively "promote" the Spanish locale to users. If it's unset (the default), we don't show a language toggle in the nav bar, and we always prefix ambiguous routes with /en/ instead of checking to see if the browser's preferred language is Spanish. The incomplete Spanish localization will still be accessible at /es/ but users will need to know that URL exists in order to visit it--we never link to it from anywhere, and we never redirect users there.

Weird technical issues

Yarn emits the following additional warnings now:

warning " > @lingui/cli@2.8.3" has unmet peer dependency "babel-plugin-macros@^2.4.2".
warning " > @lingui/macro@2.8.3" has unmet peer dependency "babel-plugin-macros@^2.5.1".

The first one is weird because we do have babel-plugin-macros 2.5.0 via react-scripts, which should satisfy that.

In any case, I tried fixing this via yarn resolutions in our package.json:

  "resolutions": {
    "react-scripts/babel-preset-react-app/babel-plugin-macros": "2.5.1"
  },

However, this just replaces the previous warnings with this one:

warning Resolution field "babel-plugin-macros@2.5.1" is incompatible with requested version "babel-plugin-macros@2.5.0"

So I just left things as-is for now, since everything seems to be working okay. I think once we upgrade to the latest version of Create React App, these warnings will probably go away, but we should definitely keep our eyes on it.

A note on canonical URLs

I wasn't sure if our localized routes should actually have a <link rel="canonical"> that points to their non-localized equivalent (e.g. /en/ and /es/ having a <link rel="canonical" href="/"> on it). It looks like this has been asked on StackOverflow and there's no clear consensus. But I also looked at the HTML for mozilla.org and it specifies the canonical URL as being locale-prefixed, so it doesn't seem like it will be an issue for us (in other words, we'll treat the locale-specific version of each page as an actual separate URL, not as just an alias for a canonical locale-independent URL).

To do

  • Use navigator.language to detect what the default locale should be. (We could do this later, of course.)
  • Add some mechanism to switch the language. Since we will likely only have two for a while (English and Spanish) we might want to just have a link that says "English" or "Espanol" appear in the navbar, depending on the current locale, where clicking on the link takes the user to the same page but in the other language.
  • Feature-flag this so we can merge this into master without having to translate everything to Spanish first.
  • Use lingui's JS API for the data visualization in IndicatorsViz.js to make sure lingui's JS API works as expected.
  • See if we can remove the <span> that wraps the site's localized title text.
  • Try translating a string that requires us to use lingui's <Plural> component, to make sure that works as expected.
  • File an issue to use Helmet to set the correct lang attribute on the page's <html> element once we've migrated to React 16 (Upgrade to React 16 #188). (When I tried using Helmet in this codebase resulted in a weird error very similar to <Trans> component's render() method returns a string, which causes React 15.6.1 to fail lingui/js-lingui#592, which is what made me realize the errors might have had something to do with our version of React.)
  • File an issue to properly localize the diagram in the "How it works" page. It looks like the original OmniGraffle source file is on Dropbox.

@toolness toolness changed the title [WIP EXPERIMENT] Add lingui support with localized pathnames [WIP] Add lingui support with localized pathnames Nov 20, 2019
@toolness toolness changed the title [WIP] Add lingui support with localized pathnames Add lingui support with localized pathnames Nov 21, 2019
@toolness
Copy link
Contributor Author

Ok @sraby, I think this is ready to go, can you review and then I'll squash merge?

@toolness toolness mentioned this pull request Nov 22, 2019
@toolness toolness changed the title Add lingui support with localized pathnames Add foundational internationalization support via lingui Nov 22, 2019
@toolness
Copy link
Contributor Author

[ch606]

@sraby
Copy link
Member

sraby commented Nov 26, 2019

Looks great @toolness! Going to merge it now.

Do you think the peer dependency warnings have anything to do with the fact that we have separate package.json configs for both the root directory and client?

@sraby sraby merged commit ade000b into master Nov 26, 2019
@sraby sraby deleted the lingui-with-paths branch November 26, 2019 14:31
toolness added a commit to JustFixNYC/justfix-website that referenced this pull request Aug 13, 2020
This factors out a <LocaleLink> component that automatically prefixes the passed-in URL with the current locale, as in WoW (see JustFixNYC/who-owns-what#186).
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