-
Notifications
You must be signed in to change notification settings - Fork 152
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
feat(i18n): Setup react-i18next #9797
Conversation
export const PLACEHOLDER = "Search by artist, gallery, style, theme, tag, etc." | ||
export const PLACEHOLDER_XS = "Search Artsy" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These weren't being used anywhere besides inside the SearchBar
component, so we deleted it from here.
src/v2/System/i18n/i18n.ts
Outdated
debug: true, | ||
// backend: { | ||
// loadPath: "assets/locales/{{lng}}/{{ns}}.json", | ||
// }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ideally, we would like the JSON files to be loaded like this, but we couldn't make it work (serving the static assets using webpacker... any ideas?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we tried a few options but none worked, basically, we need to serve the locale JSON files somewhere, we tried under /assets but got lost in the setup of how to do it, any suggestions? cc @damassi 😬
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to pair on this when y'all have a moment to pick this back up!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @damassi, that would be great! When would be a good moment for you?
Also: is there something that should be prepared beforehand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm back from OOO; please reach out in slack!
@@ -1,5 +1,6 @@ | |||
import { HelpIcon } from "@artsy/palette" | |||
import { Tooltip } from "v2/Components/Tooltip" | |||
import "v2/System/i18n/i18n" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We actually wanted that this import
would happen only on the Boot
component and it shouldn't be needed anywhere else. However, the components inside the stitch_components
directory are used in the .jade
files for rendering some of our static pages (like /terms
, for example), which don't rely on Boot
- this is why we need to replicate this import
in some of these files.
See also this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't worry about anything that exists inside of /desktop
; this folder is soon going to be deleted completely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm, but it's still being used right? The issue here is that if we don't make this change here, some of our existing pages crash...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh i see! ok, np 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once we delete desktop this will just go away safely
d85c8f0
to
fcb97f2
Compare
fcb97f2
to
0042ad0
Compare
264dff2
to
dfd83be
Compare
This setting was leading our json files to be misinterpreted by the transpiler (build would fail trying to read json files as js files)
b680848
to
e4f424e
Compare
jest.config.v2.js
Outdated
@@ -19,7 +19,7 @@ module.exports = { | |||
testURL: "http://localhost", | |||
transform: { | |||
"\\.(gql|graphql)$": "jest-transform-graphql", | |||
".(ts|tsx|js|jsx)": "babel-jest", | |||
"^.+\\.(ts|tsx|js|jsx)$": "babel-jest", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this regex feels weird.. the $
at the end makes sense, and should be enough to exclude the json files. the beginning part seems unnecessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Applied here: b5a4584
const translations = require("v2/System/locales/en-US/translation.json") | ||
|
||
i18n.use(initReactI18next).init({ | ||
debug: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this true
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought it would be better to keep it true
as it is helpful in case we have issues when translations can't be loaded (default value is false). Would it be better to remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as long as it doesnt break anything for real user on prod, then lets keep it here. I'm just not sure what this true
entails, so I was being cautious.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call!
This is the official definition: https://www.i18next.com/overview/configuration-options
logs info level to console output. Helps finding issues with loading not working
As I understand, it's just about logs, so I don't think it can break anything. Do you think I could be missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense. all good then :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should only enable this in development, which is as easy as checking debug: getENV("NODE_ENV") === "development"
@@ -149,7 +151,7 @@ export const NavBar: React.FC = track( | |||
flex={1} | |||
size="small" | |||
> | |||
Sign Up | |||
{t`navbar.signup`} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice
@@ -62,17 +62,17 @@ describe("NavBar", () => { | |||
describe("desktop", () => { | |||
it("renders logged out items", () => { | |||
const wrapper = getWrapper() | |||
expect(wrapper.html()).toContain("Log In") | |||
expect(wrapper.html()).toContain("Sign Up") | |||
expect(wrapper.html()).toContain("navbar.login") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is strange.. should it contain the key? feels like the test should be testing if the actual text is there.
something like
it("works in English", () => {
expect(getWrapper().html()).toContain("Log In")
})
it("works in Greek", () => {
someFuncToMockBrowserLang("gr")
expect(getWrapper().html()).toContain("Σύνδεση")
})
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is something that we also discussed during the implementation... but if the goal here is that at some point the language experts should be capable of using a third-party service to update the translations as they wish, do we want the tests to fail when the content of text changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i would say yes 🤔.
i guess we should support both? it would be great to be able to test the actual text we expand to, to test that languages are actually applied correctly, but also we should allow for key strings to be checked, though that can just be the testID
🤔. Im not set on one or the other.
Maybe we start with something and see how it goes. I just feel like checking just for the key might take us to a place where a translation is missing, and we never know, because our test is green, and our website says navbar.signup
instead of Sign Up
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this is a super valid point
I added 1b51258 to change the test to look for the strings instead of keys
|
||
const logger = createLogger("Components/Search/SearchBar") | ||
|
||
export interface Props extends SystemContextProps { | ||
export interface Props extends SystemContextProps, WithTranslation { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why would we do this here this way, and not with the same useTranslation
hook?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh i see.. this is a class component.
even though it makes sense, I would much prefer it if we used something like https://github.com/artsy/eigen/blob/26a98c408c223efefbb0b8309448c3bb79256ab3/src/app/Scenes/Search/components/CityGuideCTA.tsx#L12-L13 (docs here at the bottom https://github.com/artsy/eigen/blob/b4484db7fb7ddfbad4e903c993628bbd90a909b3/docs/palette_v3_migration_guide.md)
so we can keep everything nice and centralized, with a t
exposed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @pvinis !! ❤️
As this is not part of an actual project, I don't think I'll have time to focus a lot on these changes before Wednesday. Do you think it would make sense to remove the SearchBar
translations from this PR so we could make it smaller by applying the i18n to the NavBar
only? And then working on the SearchBar on the next Future Friday?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you think the way I suggest is a good way to do this change for the file here, I'm happy to help or even make a PR on top of this one with the change. If you think we need more discussion on this, I'll gladly greenlight this 👍.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I completely agree that your suggestion would be much cleaner, if it's possible to open a PR on top of this one, that would be great. I just don't want to put any pressure on you either, I'm already super grateful for the careful code review. ❤️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll take a quick stab at it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some questions and some suggestions 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
approving this, since the comments are non-blocking. happy to pair or make a PR on top of this for a <ClassI18n>
component to simplify the class comp case.
* simplifying * chiller types
Thank you so much for the PR, @pvinis, I'm merging this now ❤️ |
react-i18nextAuthor: Jan Mühlemann Description: Internationalization for react done right. Using the i18next i18n ecosystem. Homepage: https://github.com/i18next/react-i18next
insaneAuthor: Nicolas Bevacqua Description: Lean and configurable whitelist-oriented HTML sanitizer Homepage: https://github.com/bevacqua/insane
|
Created | over 6 years ago |
Last Updated | about 1 month ago |
License | MIT |
Maintainers | 2 |
Releases | 49 |
Direct Dependencies | @babel/runtime |
Keywords | i18next and i18next-languageDetector |
README
Introduction
This is a i18next language detection plugin use to detect user language in the browser with support for:
- cookie (set cookie i18next=LANGUAGE)
- sessionStorage (set key i18nextLng=LANGUAGE)
- localStorage (set key i18nextLng=LANGUAGE)
- navigator (set browser language)
- querystring (append
?lng=LANGUAGE
to URL) - htmlTag (add html language tag <html lang="LANGUAGE" ...)
- path (http://my.site.com/LANGUAGE/...)
- subdomain (http://LANGUAGE.site.com/...)
Getting started
Source can be loaded via npm, bower or downloaded from this repo.
# npm package
$ npm install i18next-browser-languagedetector
# bower
$ bower install i18next-browser-languagedetector
- If you don't use a module loader it will be added to
window.i18nextBrowserLanguageDetector
Wiring up:
import i18next from 'i18next';
import LanguageDetector from 'i18next-browser-languagedetector';
i18next.use(LanguageDetector).init(i18nextOptions);
As with all modules you can either pass the constructor function (class) to the i18next.use or a concrete instance.
Detector Options
The default options can be found here.
{
// order and from where user language should be detected
order: ['querystring', 'cookie', 'localStorage', 'sessionStorage', 'navigator', 'htmlTag', 'path', 'subdomain'],
// keys or params to lookup language from
lookupQuerystring: 'lng',
lookupCookie: 'i18next',
lookupLocalStorage: 'i18nextLng',
lookupSessionStorage: 'i18nextLng',
lookupFromPathIndex: 0,
lookupFromSubdomainIndex: 0,
// cache user language on
caches: ['localStorage', 'cookie'],
excludeCacheFor: ['cimode'], // languages to not persist (cookie, localStorage)
// optional expire and domain for set cookie
cookieMinutes: 10,
cookieDomain: 'myDomain',
// optional htmlTag with lang attribute, the default is:
htmlTag: document.documentElement,
// optional set cookie options, reference:[MDN Set-Cookie docs](https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Set-Cookie)
cookieOptions: { path: '/', sameSite: 'strict' }
}
Options can be passed in:
preferred - by setting options.detection in i18next.init:
import i18next from 'i18next';
import LanguageDetector from 'i18next-browser-languagedetector';
i18next.use(LanguageDetector).init({
detection: options,
});
on construction:
import LanguageDetector from 'i18next-browser-languagedetector';
const languageDetector = new LanguageDetector(null, options);
via calling init:
import LanguageDetector from 'i18next-browser-languagedetector';
const languageDetector = new LanguageDetector();
languageDetector.init(options);
Adding own detection functionality
interface
export default {
name: 'myDetectorsName',
lookup(options) {
// options -> are passed in options
return 'en';
},
cacheUserLanguage(lng, options) {
// options -> are passed in options
// lng -> current language, will be called after init and on changeLanguage
// store it
},
};
adding it
import LanguageDetector from 'i18next-browser-languagedetector';
const languageDetector = new LanguageDetector();
languageDetector.addDetector(myDetector);
i18next.use(languageDetector).init({
detection: options,
});
Don't forget: You have to add the name of your detector (myDetectorsName
in this case) to the order
array in your options
object. Without that, your detector won't be used. See the Detector Options section for more.
Gold Sponsors
localization as a service - locize.com
Needing a translation management? Want to edit your translations with an InContext Editor? Use the orginal provided to you by the maintainers of i18next!
With using locize you directly support the future of i18next and react-i18next.
i18next
Author: Jan Mühlemann
Description: i18next internationalization framework
Homepage: https://www.i18next.com
Created | over 10 years ago |
Last Updated | 12 days ago |
License | MIT |
Maintainers | 2 |
Releases | 381 |
Direct Dependencies | @babel/runtime |
Keywords | i18next, internationalization, i18n, translation, localization, l10n, globalization and gettext |
README
i18next: learn once - translate everywhere
i18next is a very popular internationalization framework for browser or any other javascript environment (eg. Node.js, Deno).
i18next provides:
- Flexible connection to backend (loading translations via xhr, ...)
- Optional caching, user language detection, ...
- Proper pluralizations
- Translation context
- Nesting, Variable replacement
- Flexibility: Use it everywhere
- Extensibility: eg. sprintf
- ...
For more information visit the website:
Our focus is providing the core to building a booming ecosystem. Independent of the building blocks you choose, be it react, angular or even good old jquery proper translation capabilities are just one step away.
Documentation
The general i18next documentation is published on www.i18next.com and PR changes can be supplied here.
The react specific documentation is published on react.i18next.com and PR changes can be supplied here.
Gold Sponsors
From the creators of i18next: localization as a service - locize.com
A translation management system built around the i18next ecosystem - locize.com.
With using locize you directly support the future of i18next.
New dependencies added: i18next
, i18next-browser-languagedetector
, insane
and react-i18next
.
🎉 |
We setup react-i18next during [these changes](#9797), these docs contain instructions about the context, usage and next steps. Co-authored-by: Lidiane Taquehara <lidi.mayra@gmail.com> Co-authored-by: Leandro Linares <leandro.linares@artsymail.com>
We setup react-i18next during [these changes](#9797), these docs contain instructions about the context, usage and next steps. Co-authored-by: Lidiane Taquehara <lidi.mayra@gmail.com> Co-authored-by: Leandro Linares <leandro.linares@artsymail.com>
We setup react-i18next during [these changes](#9797), these docs contain instructions about the context, usage and next steps. Co-authored-by: Lidiane Taquehara <lidi.mayra@gmail.com> Co-authored-by: Leandro Linares <leandro.linares@artsymail.com>
* Include docs about i18n We setup react-i18next during [these changes](#9797), these docs contain instructions about the context, usage and next steps. Co-authored-by: Lidiane Taquehara <lidi.mayra@gmail.com> Co-authored-by: Leandro Linares <leandro.linares@artsymail.com> * Move locales to /i18n directory * Add comment about assets served on the server-side * camelCase instead of lowerCamelCase * Update docs/i18n.md Co-authored-by: Pavlos Vinieratos <pvinis@gmail.com> Co-authored-by: Leandro Linares <leandro.linares@artsymail.com> Co-authored-by: Pavlos Vinieratos <pvinis@gmail.com>
Setup i18n on Force using react-i18next.
For questions/suggestions, join #hack9-localizations slack channel
For context and usage, see docs available at https://github.com/artsy/force/blob/main/docs/i18n.md