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.

Sign up for GitHub

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

Jump to bottom

Internationalization approach does not support languages with different word orders #2069

Closed
timabbott opened this issue Feb 12, 2020 · 2 comments · Fixed by #2077
Closed

Internationalization approach does not support languages with different word orders #2069

timabbott opened this issue Feb 12, 2020 · 2 comments · Fixed by #2077
Assignees
@goto-bus-stop
Labels
Bug 🌏 Locale Translations and internationalization

Comments

@timabbott
Copy link

We're looking at using Uppy for Zulip, and overall it looks great.

However, the internationalization approach for Uppy appears to be based on string concatenation, which means that it can't support languages with a different word order (where e.g. the maximum file size would not be the last word). See for example https://github.com/transloadit/uppy/blob/master/packages/%40uppy/core/src/index.js#L460:

throw new RestrictionError(`${this.i18n('exceedsSize')} ${prettyBytes(maxFileSize)}`)

The right way to do this would be something with an API like this ${this.i18n('exceedsSize, ${prettyBytes(maxFileSize)}')} (and have the string have a {file_size} variable in it).

See https://zulip.readthedocs.io/en/latest/translating/internationalization.html#key-details-about-human-language for more background and examples of how other software handles i18n.
@timabbott timabbott mentioned this issue Feb 12, 2020
Merged
7 tasks
@timabbott
Copy link
Author

Oh, reading the code more closely, your i18n library supports it; it's just this string that isn't tagged properly. So you should be able to just change that line to roughly this:

          throw new RestrictionError(this.i18n('exceedsSize'), {                                     
              max_file_size: prettyBytes(maxFileSize),                                               
          });

@kvz kvz mentioned this issue Feb 13, 2020
Open
@goto-bus-stop
Copy link
Contributor

Yeah, I think this is a problem in a couple of places. Fixing it would be a breaking change technically. Perhaps we could add a new string like exceedsSize2 with the placeholder and use the old way if that doesn't exist…

@goto-bus-stop goto-bus-stop added 🌏 Locale Translations and internationalization and removed Triage labels Feb 17, 2020
goto-bus-stop added a commit that referenced this issue Feb 17, 2020
@goto-bus-stop
Fix translations that did not respect word order
0b702dd 
Fixes #2069

We were still using raw string concatenation in these two places. To
maintain backwards compatibility, this commit adds _new_ translations
that are substitution-based instead of concatenation-based. The old
translation is passed in as a possible substitution parameter
`backwardsCompat`, and the default (English) translations use this
parameter.

With this, if you are using a custom locale that overrides eg. the
`exceedsSize` string, Uppy will first get your `exceedsSize`
translation, and then use the default `exceedsSize2` translation which
concatenates that result and the file size. So it works the same as it
did in the past.
If a translation requires a different word order, it can override the
`exceedsSize2` translation instead.
```js
new Uppy({
  locale: {
    strings: {
      exceedsSize2: 'Maximum file size of %{size} exceeded',
      poweredBy2: '%{uppy} made this!'
    }
  }
})
```

In 2.0, we can remove all the old strings and only use `poweredBy2` (and
probably rename them to `poweredBy` too, heh)
@goto-bus-stop goto-bus-stop mentioned this issue Feb 17, 2020
Merged
@goto-bus-stop goto-bus-stop self-assigned this Feb 17, 2020
arturi added a commit that referenced this issue Mar 19, 2020
@goto-bus-stop @arturi
Fix translations that did not respect word order (#2077)
5e25b2a 
* Fix translations that did not respect word order

Fixes #2069

We were still using raw string concatenation in these two places. To
maintain backwards compatibility, this commit adds _new_ translations
that are substitution-based instead of concatenation-based. The old
translation is passed in as a possible substitution parameter
`backwardsCompat`, and the default (English) translations use this
parameter.

With this, if you are using a custom locale that overrides eg. the
`exceedsSize` string, Uppy will first get your `exceedsSize`
translation, and then use the default `exceedsSize2` translation which
concatenates that result and the file size. So it works the same as it
did in the past.
If a translation requires a different word order, it can override the
`exceedsSize2` translation instead.
```js
new Uppy({
  locale: {
    strings: {
      exceedsSize2: 'Maximum file size of %{size} exceeded',
      poweredBy2: '%{uppy} made this!'
    }
  }
})
```

In 2.0, we can remove all the old strings and only use `poweredBy2` (and
probably rename them to `poweredBy` too, heh)

* website: add missing dashboard strings to docs

* locales: remove unused second plural form from EN and NL

* build: allow different numbers of plural forms

* utils: add test for new throwing behaviour

* docs: insert PR url

* locales: revert ko_KR changes

* changelog: add 2.0 backlog entry

* locales: var → const

* docs: add brief note that translators should supply a pluralize function

* locales: update en_US with new template

* fix HEIGHT_MD = 400

Co-authored-by: Artur Paikin <artur@arturpaikin.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@goto-bus-stop goto-bus-stop

Labels
Bug 🌏 Locale Translations and internationalization
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix translations that did not respect word order transloadit/uppy
2 participants
@goto-bus-stop @timabbott