Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

i18n string dictionaries #3532

Merged
merged 47 commits into from
Dec 11, 2016
Merged

i18n string dictionaries #3532

merged 47 commits into from
Dec 11, 2016

Conversation

jacogr
Copy link
Contributor

@jacogr jacogr commented Nov 18, 2016

  • Settings
  • TabBar

Testing i18n React implementation

parity 2016-11-22 14-23-20

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M7-ui labels Nov 18, 2016
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Nov 22, 2016
@jacogr
Copy link
Contributor Author

jacogr commented Nov 22, 2016

@ngotchac & @derhuerst Comments welcome.

@derhuerst
Copy link
Contributor

Improved the German translations, but haven't looked at the code yet.

@gavofyork
Copy link
Contributor

Qt's i18n support is light-years ahead of Javascript, it seems.

@jacogr
Copy link
Contributor Author

jacogr commented Nov 23, 2016

Sadly, yes, especially when it comes to React + JS. (Then again, I did Qt i18n around 2001, so it has had a lot of time.) It is basically only a dictionary of strings, with replacements, i.e. can do "you have %{count} messages". But overall could whip this up, in this form, in an afternoon.

There are some things not in here with regards date formatting (as well as currencies) and plurals, but once again, nothing to write home about. There is also the Yahoo React i18n package, which is in the same vein (slightly stronger with actual formatting).

@tomusdrw
Copy link
Collaborator

There should be something gettext based for React, I was using couple of nice tools for angular (including extracting translations to pot files and automatically parsing po)

// You should have received a copy of the GNU General Public License
// along with Parity. If not, see <http://www.gnu.org/licenses/>.

import { initLocales, getLocale, setLocale } from './i18n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not move i18n/i18n.js to i18n/index.js?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could work and make it simpler.

@@ -29,6 +29,9 @@ import qs from 'querystring';

import SecureApi from './secureApi';
import ContractInstances from './contracts';
import { initLocales } from './i18n';

initLocales('en');
Copy link
Contributor

Choose a reason for hiding this comment

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

I find the naming initLocales confusing. It sounds as if you could pass one or more locales to be initialised, in this case only English. However, underneath, all locales will be initialised, with English being the chosen one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, suggestions?

function flattenObject (localeObject) {
return Object
.keys(localeObject)
.reduce((obj, key) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Tiny grumble: I'd prefer more expressive var names like obj -> all, key -> ns/base, flatKey -> key.

Also, consider a module like: flat. I handles edge cases like arrays (typeof [] === 'object') and is well-tested.

But this is far from blocking this PR. (:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Should be removed and inputs done properly as per the defaultMessage output file. (See later comment.)

@observable isDevelopment = !isProduction;

constructor () {
addLocaleData([...de, ...en]);
Copy link
Contributor

@derhuerst derhuerst Nov 30, 2016

Choose a reason for hiding this comment

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

Really not sure here…

This should probably get called only once during runtime, as it has static arguments anyway. If I created two stores, it would get called twice. What about moving it up, below LOCALES?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed.

<div>
<FormattedMessage
id='settings.background.overview_0'
defaultMessage='The background pattern you can see right now is unique to your Parity installation. It will change every time you create a new Signer token. This is so that decentralized applications cannot pretend to be trustworthy.' />
Copy link
Contributor

Choose a reason for hiding this comment

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

If we have a defaultMessage (which I think is good for readability), we do we need a separate en i18n file? They will probably not stay consistent over time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, don't think I explained it properly. Will bring in the babel/webpack plugin to extract the messages, i.e. you can generate a file from the defaultMessages at build and use this as an input into the translation.

(There are some text still in the English file, since they are used at multiple places, same text, same reference, just duplicated - kept it that way for now since it removes the duplication. Needs to be put back, not duplicated)

@derhuerst derhuerst mentioned this pull request Nov 30, 2016
@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 1, 2016
# Conflicts:
#	ethcore/light/src/types/mod.rs
#	js/package.json
#	js/src/i18n/en/index.js
#	js/src/modals/SMSVerification/GatherData/gatherData.js
#	js/src/modals/SMSVerification/SendConfirmation/sendConfirmation.js
#	js/src/modals/SMSVerification/SendRequest/sendRequest.js
#	js/src/ui/index.js
#	js/src/util/nullable-proptype.js
#	js/src/util/proptypes.js
#	js/src/views/Settings/Parity/parity.js
@jacogr jacogr added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 8, 2016
@jacogr jacogr merged commit 885d6ea into master Dec 11, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants