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

feat: add translation support #2870

Merged
merged 5 commits into from
Nov 14, 2019
Merged

feat: add translation support #2870

merged 5 commits into from
Nov 14, 2019

Conversation

erezrokah
Copy link
Contributor

@erezrokah erezrokah commented Nov 12, 2019 •
edited
Loading

Fixes #789

Inspired by #2528 and #2278.

Similar mechanism as https://github.com/date-fns/date-fns/blob/aa1daea6a8a1493429330cdd82ae7c325a97b902/src/index.js

@erquhart I added a new package for the locales, I'm not sure how to go about versioning and publishing it

This was referenced Nov 12, 2019
Closed
Closed
@barthc
Copy link
Contributor

barthc commented Nov 12, 2019

I assume the new package netlify-cms-locales would be bundled together with the app, no ?. Could increase bundle size as we add more locales.
Based on this comment #789 (comment) I am thinking we are looking for something similar to the way we load external config via url.

@erezrokah
Copy link
Contributor Author

erezrokah commented Nov 12, 2019 •
edited
Loading

Hey @barthc. Not necessarily - in case you're using Webpack with the netlify-cms-app non registered locales will be tree shaked similar to the external media libraries: https://www.netlifycms.org/blog/2019/07/netlify-cms-gatsby-plugin-4-0-0/#using-media-libraries-with-netlify-cms-app

@chrfritsch
Copy link
Contributor

I like the approach here. I also like that all supported languages are managed inside this repo. That makes contributing new translations quite easy.

I will open a PR later today to provide the german translations.

@chrfritsch
Copy link
Contributor

During working on the german translation I realized that translating widget strings is currently not possible. For example "Choose an image"

@chrfritsch
Copy link
Contributor

Here is the PR with the german translation #2877

@erezrokah
Copy link
Contributor Author

During working on the german translation I realized that translating widget strings is currently not possible. For example "Choose an image"

Can you please open a separate issue for it?
Looks like this is the relevant place in the code to fix it: https://github.com/netlify/netlify-cms/blob/4833f337280dee0105b282c4863b3b674c818cba/packages/netlify-cms-widget-file/src/withFileControl.js#L249

erquhart
erquhart reviewed Nov 14, 2019
View reviewed changes
Copy link
Contributor

@erquhart erquhart left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Solid, I'm pretty much good with this as is. My only thought besides my review comment is to register all non-en locales (so de currently) in the netlify-cms package. The pattern thus far has been to let that package "just work" so only configuration changes are needed to enable functionality, at the expense of extra script size. As mentioned before, I expect that tech debt to be paid with lazy loading in the future.

packages/netlify-cms-app/package.json Outdated
@@ -49,6 +49,7 @@
"netlify-cms-widget-select": "^2.4.3",
"netlify-cms-widget-string": "^2.2.3",
"netlify-cms-widget-text": "^2.2.3",
"netlify-cms-locales": "^0.0.1",
Copy link
Contributor

Choose a reason for hiding this comment

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

I usually start new packages at 1.0.0, partly for parity and partly because the semver caret range only gets patch releases for pre-1.0 packages. I'm also cool with just using a different range that will get feature bumps and keeping it at 0.0.1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll change it to 1.0.0

@erezrokah
Copy link
Contributor Author

Updated the package version and registered all locales for netlify-cms.
The english translation size is 6.4K (2K gzipped) so I wouldn't worry about bundle size atm.
Also this #2875 just shaved a 100K (unzipped) off the bundle.

@erezrokah erezrokah merged commit 096b067 into master Nov 14, 2019
@erezrokah erezrokah mentioned this pull request Nov 14, 2019
Closed
@erquhart erquhart deleted the feat/translation_support branch November 14, 2019 19:43
MilesRomney pushed a commit to MilesRomney/netlify-cms that referenced this pull request Nov 18, 2019
@erezrokah @MilesRomney
feat: add translation support (decaporg#2870)
9ff04e1 
* feat: add translation support

* test(cypress): fix locale import

* docs: add locale documentation

* feat: add german translation (decaporg#2877)

* fix: locales package version, register all locales in netlify-cms
nathankitchen pushed a commit to nathankitchen/netlify-cms-backend-azure that referenced this pull request Feb 24, 2020
@erezrokah
feat: add translation support (decaporg#2870)
fc32b92 
* feat: add translation support

* test(cypress): fix locale import

* docs: add locale documentation

* feat: add german translation (decaporg#2877)

* fix: locales package version, register all locales in netlify-cms
vladdu pushed a commit to vladdu/netlify-cms that referenced this pull request Jan 26, 2021
@erezrokah @vladdu
feat: add translation support (decaporg#2870)
a2537ee 
* feat: add translation support

* test(cypress): fix locale import

* docs: add locale documentation

* feat: add german translation (decaporg#2877)

* fix: locales package version, register all locales in netlify-cms
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erquhart erquhart erquhart left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Translation
4 participants
@erezrokah @barthc @chrfritsch @erquhart