Skip to content

LG-3175: Create localization utilities for React components#3909

Merged
aduth merged 1 commit intomasterfrom
aduth-lg-3175-react-i18n
Jul 15, 2020
Merged

LG-3175: Create localization utilities for React components#3909
aduth merged 1 commit intomasterfrom
aduth-lg-3175-react-i18n

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 13, 2020

Why: As a user, I want pages that I'm viewing to be localized to my language, including those page which use React.

This pull request adds simple localization utilities for React components. It is implemented using React context to establish localization data at the root of the application. A custom React hook is included for the purpose of simplifying usage, modeled after existing LoginGov.I18n.t and server-side t Rails internationalization.

Making this data available via context should hopefully facilitate better unit testing, since it doesn't require mocking of window globals, but instead allows for any (or no) simple JavaScript object to be provided as localization data. The current implementation of the translation utility will return the given key if there is no localized data. While this is not appropriate for the front-end user experience, it could be a fine compromise for tests to be able to find elements by a given string key.

The alternative would be to find some way to make the localization data available to the tests. This would have the advantage of bringing tests closer to a "real" user experience of testing components by text visible in the page, but would require additional test bootstrapping logic. The logic would likely involve either loading all YAML files from config/locales as a plain JavaScript object, or as a prerequisite build artifact generated by Rails (see also: i18n-js export, react_on_rails I18n).

Base automatically changed from aduth-lg-3178-react-testing to master July 13, 2020 15:57
@aduth aduth mentioned this pull request Jul 13, 2020
@aduth aduth force-pushed the aduth-lg-3175-react-i18n branch from a15bafc to cb7497b Compare July 13, 2020 20:07
@aduth
Copy link
Contributor Author

aduth commented Jul 14, 2020

There's currently an error being reported in CodeClimate, but it's not one I would expect to see:

CodeClimate error

The parentheses are expected:

Unfortunately, CodeClimate's web UI doesn't give much debugging information to work from.

I installed and ran their CLI tool and was able to reproduce the error.

I observed from their documentation that they run a very outdated version of ESLint by default (3.x, currently 7.x). I suspected it might be something where the ESLint configuration is not able to be loaded, and instead is falling back to some default.

Configuring CodeClimate locally to use a newer version of ESLint appears to help make the difference:

$ codeclimate analyze -e eslint app/javascript/app/document-capture/hooks/use-i18n.js
Starting analysis
Running eslint: Done!

== app/javascript/app/document-capture/hooks/use-i18n.js (1 issue) ==
6: Unexpected parentheses around single function argument having a body with no curly braces [eslint]

Analysis complete! Found 1 issue.
$ codeclimate analyze -e eslint:eslint-6 app/javascript/app/document-capture/hooks/use-i18n.js
Starting analysis
Running eslint: Done!

Analysis complete! Found 0 issues.

Looking through the release notes, nothing in particular stands out to me as making the difference here.

That being said, it would be wise to bring the CodeClimate ESLint configured version in line with what's installed in the application.

#3903 upgraded ESLint from 4.x to the latest 7.x version. ESLint 7 does not appear to be supported yet by CodeClimate. We can configure it as recent as using version 6. I'd recommend giving this a try, and if issues persist, we could consider to downgrade the version used in the application.

It's something which should probably be done as its own task, but since it blocks this pull request, I'd propose to commit it here.

@aduth
Copy link
Contributor Author

aduth commented Jul 14, 2020

5673f9d seemed to work, although it works a little too well 😓 Catching a bunch of preexisting lint errors now.

@aduth
Copy link
Contributor Author

aduth commented Jul 14, 2020

5673f9d seemed to work, although it works a little too well 😓 Catching a bunch of preexisting lint errors now.

Pulled out to #3914 to address separately.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

**Why**: As a user, I want pages that I'm viewing to be localized to my language, including those page which use React.
@aduth aduth force-pushed the aduth-lg-3175-react-i18n branch from f6842c9 to a1756a1 Compare July 15, 2020 19:35
@aduth aduth merged commit c6b22ff into master Jul 15, 2020
@aduth aduth deleted the aduth-lg-3175-react-i18n branch July 15, 2020 20:24
ironman5366 added a commit that referenced this pull request Jul 22, 2020
New JS code added in #3909 and others require strings configured in assets.js.erb and i18n-strings.js.erb.

The added make task, check_asset_strings, assets that for each such string referenced in a Javascript file, there exists a corresponding entry in one of the erb files
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