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

feat: i18n #772

Merged
merged 15 commits into from
Sep 6, 2018
Merged

feat: i18n #772

merged 15 commits into from
Sep 6, 2018

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Sep 1, 2018

Ref.: #675

This doesn't add translations for Explore view since it will be used as an external component. /cc @olizilla

@hacdias
Copy link
Member Author

hacdias commented Sep 2, 2018

So, I've started using react-i18next for this WebUI and I'm using namespaces to divide the translations into files so each page has its own translation file. It might be useful if we subdivide each part into a different web app.

@ipfs-shipyard/gui could you take a look before I keep going? Do you agree with this approach?

@hacdias
Copy link
Member Author

hacdias commented Sep 3, 2018

Well, the biggest file would be files.json and some, like peers and status' would have barely ten lines. Maybe it's actually better to merge them. Opinions?

@fsdiogo
Copy link
Contributor

fsdiogo commented Sep 3, 2018

I'd say to keep it in separate files anyway. In the future it could explode translation-wise and that way we'll be ready 🙂

@hacdias
Copy link
Member Author

hacdias commented Sep 3, 2018

@fsdiogo yup, that could also happen! I'll keep them separate then 😄 It will also be easier to split the Web UI into multiple smaller web apps.

@hacdias hacdias changed the title [wip] start i18n feat: i18n Sep 3, 2018
@hacdias
Copy link
Member Author

hacdias commented Sep 3, 2018

I think this is basically done. Doesn't seem to be any string left.

Copy link
Member

@lidel lidel left a comment

Choose a reason for hiding this comment

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

I think the layout is good: all namespaces are kept in public/locales/en which works well as a source for PR-based translations, and namespacing makes it easier for people to submit translations via PRs.

Just for the record, there is something to be aware when it comes to i18next: IIUC the project works out-of-the-box only with their own translation website, https://locize.com: i18next/react-i18next#472 (comment).

I was unable to find native support for i18next translation format at Transifex (which I am leaning towards, here is why). People were successful with using Transifex+i18next-gettext-converter which has a support for plurals for most of languages, but there are edge cases with known issues which may produce frustrating bugs. I feel we could try to blackbox the sync by creating 1 shell script that takes care of conversion and runs at jenkins + is run manually before release, but this need for additional layer of indirection if a sort-of a warning flag.

Update: nvm, i18next supports ICU after all!

The alternative, react-intl is a part of FormatJS which is aligned with ECMAScript Internationalization API (ECMA-402), Unicode CLDR, and ICU Message syntax. ICU does not require additional conversion step and is supported out of the box by Transifex and other crowdsourcing websites. It kinda feels more future-proof, when looked at from that angle, but I am not sure if its enough to move back.

@hacdias did i18next made a big improvement over react-int for you as a dev?
If we consolidate translations under GUI of Transifex, then the "ugliness" of ICU format may not be that big of a deal – translators will not use it directly, and use of industry standard may save us some headache in the long run.

@hacdias
Copy link
Member Author

hacdias commented Sep 5, 2018

@lidel @olizilla should I start working on moving this to ICU format already?

@olizilla
Copy link
Member

olizilla commented Sep 5, 2018

@hacdias what are your thoughts on @lidel's question:

did i18next made a big improvement over react-int for you as a dev?
If we consolidate translations under GUI of Transifex, then the "ugliness" of ICU format may not be that big of a deal – translators will not use it directly, and use of industry standard may save us some headache in the long run.

@hacdias
Copy link
Member Author

hacdias commented Sep 5, 2018

@olizilla for me, react-i18next was much easier to use and felt more natural. Maybe I tried react-intl the hardest way. One thing I didn't like about it was this:

 <FormattedMessage
                    id="welcome"
                    defaultMessage={`Hello {name}, you have {unreadCount, number} {unreadCount, plural,
                      one {message}
                      other {messages}
                    }`}
                    values={{name: <b>{name}</b>, unreadCount}}
                />

Do you see the defaultMessage there? We need to pass it through props to the component! That way the English translation wouldn't go to a JSON file. It would be scattered across our source files.

With i18next, so far, so good.

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Sep 5, 2018

@lidel @olizilla just moved the formats to ICU. It was actually a simple migration since we didn't use much plurals. Take a look at the locales directory and see how they are.

I'm up to merging this PR. Feedback?

EXPLORE is not translated.

I'd like to do a PT translation after this is merged.

@lidel
Copy link
Member

lidel commented Sep 6, 2018

Thank you @hacdias ! It is 👌 and I managed to set up Transifex as well: added the configuration file for Transifex (.tx/config) and uploaded existing source files to the website: https://www.transifex.com/ipfs/ipfs-webui

If you want we can smoke-test translation process before we merge this.:

  • Start Translating
    1. Create Transifex account
    2. Go to https://www.transifex.com/ipfs/ipfs-webui/translate/#pt and try translating some strings for PT locale
      • ping me if you don't have access to the team
  • Sync Translations
    1. Install and set up command-line client (tx) – see docs below
    2. To download new translations from Transifex: tx pull -a
      • this should create/update files in public/locales/* that need to be commited
      • if a new language is created, remember to add it to src/i18n.js
    3. (optional) upload updated source locale (en): tx push -s
      • (I already did that for this branch, so you can skip it)
      • This will be automated after revamp is moved to master, for now it is a manual step

Transifex 101

Transifex does not support full ICU when it is in JSON format,
this commit changes it to conform to one described at:
https://docs.transifex.com/formats/json#plurals-support
"descriptionFolder": "Choose a new name for this folder."
},
"deleteModal": {
"titleItem": "{count, plural, one {Delete Item} other {Delete {count} Items}}",
Copy link
Member

@lidel lidel Sep 6, 2018

Choose a reason for hiding this comment

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

FYI I had to tweak the ICU format used for plurals because Transifex supports only a subset of ICU when JSON files are used (I think the full support is in gettext):

Anyway, it now renders correctly. Below is an example of Transifex translation UI for Polish, where we have more than two plural forms 🙃

screenshot_18

Copy link
Member Author

Choose a reason for hiding this comment

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

Cool! Thanks 😄

"breadcrumbs": "Breadcrumbs",
"finished": "Finished!",
"totalSize": "Total size: {size}",
"filesSelected": "{count, plural, one {File selected} other {Files selected}}",
Copy link
Member

Choose a reason for hiding this comment

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

I also changed this one, because it supported only 2 plural forms, now it is universal :)

License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
License: MIT
Signed-off-by: Henrique Dias <hacdias@gmail.com>
@hacdias
Copy link
Member Author

hacdias commented Sep 6, 2018

@lidel I think we're ready for now 😄 Let's merge it?

@hacdias hacdias merged commit 9598b0c into revamp Sep 6, 2018
@hacdias hacdias deleted the i18n branch September 6, 2018 11:28
@olizilla
Copy link
Member

olizilla commented Sep 6, 2018

💯 🌟

@hacdias hacdias mentioned this pull request Sep 6, 2018
hacdias added a commit that referenced this pull request Sep 7, 2018
@lidel I stole some of your words on #772!

Could you take a look and add/update anything if you feel to?
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.

4 participants