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

add i18n translation support #156

Merged
merged 2 commits into from
Feb 23, 2019
Merged

Conversation

jan-xyz
Copy link

@jan-xyz jan-xyz commented Feb 19, 2019

With this Draft PR I want to get early feedback on my approach for tackling #108 and I would really appreciate some feedback from @budparr, @regisphilibert and anyone else involved.

I implemented a few strings that are easily visible but that should also include most of the strings. This theme is actually quiet and there isn't much text to translate in the first place I think.

I switched to this branch for my own blog and you can see the changes live at: https://blog.jan-steinke.de

So far I only have the translations for "de"(German) and "en"(English). The multi language support build into Hugo should default to "en" if I can trust the documentation, I haven't actually checked if this is true but it should make sure that anyone using this theme with single language should be fine as well.

defaultContentLanguage sets the project’s default language. If not set, the default language will be en.

One thing I consciously removed is the support for recent_copy and read_more_copy as I understood that these are for multi language support and should be unnecessary if we build into i18n support.

Things need clarification before WIP is done:

@echarp
Copy link
Contributor

echarp commented Feb 19, 2019

This looks awesome! 👍

You don't like using "T" instead of the longer "i18n" command? Shorter but not as clear?

I'll help with french if possible.

@jan-xyz
Copy link
Author

jan-xyz commented Feb 19, 2019

Thanks, any support is appreciated! If you want make a PR to my branch with French translations. I can also see if I can give you Push rights on my branch directly, if you want.

I saw the T option but don't really have a strong opinion on it, whatever works best for the project. Maybe something else to decide on now?

@budparr
Copy link
Member

budparr commented Feb 19, 2019

Thanks, @jan-xyz! @regisphilibert has suggested we use this construction for the strings:

{{$.Param "read_more_copy" | default (i18n "readMore") }}

otherwise, this could be a breaking change for many.

@jan-xyz
Copy link
Author

jan-xyz commented Feb 20, 2019

Sounds good to me and I fixed it with your suggestion. Is there anything else that you would like to see with this?

@budparr
Copy link
Member

budparr commented Feb 20, 2019

Looks good to me. I'm going to run it locally before merging, but will try to get to that tonight. Thanks @jan-xyz!

@jan-xyz
Copy link
Author

jan-xyz commented Feb 20, 2019

No need to thank me :) I'm benefiting as well from this project.

@jan-xyz jan-xyz marked this pull request as ready for review February 20, 2019 20:38
@budparr budparr merged commit 76e4b71 into theNewDynamic:master Feb 23, 2019
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.

3 participants