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

Basic i18n implementation #60

Closed
wants to merge 17 commits into from
Closed

Basic i18n implementation #60

wants to merge 17 commits into from

Conversation

oncleben31
Copy link
Contributor

@oncleben31 oncleben31 commented Oct 6, 2016

So I've redo my try to put some basic i18n. This time it's more clean and I use yaml files to store the locales strings.

It still work in progress as there are two points on my todo list:

  • (mandatory) make it work with the locales files in the theme folder not in the example/_data folder (Edited not possible currently)
  • find an easy way to localize the dates. (Edited done)

cc/ @ashmaroli Have you any comment on that ?

@ashmaroli
Copy link
Member

Lovely, I dig the use of _data/locales/[lang].yml
There are two ways to consider..

When setting default: [params] within the liquid tag (inside _layouts/home.html) for every i18n string,
you dont need to explicitly set the site.lang: en in _config.yml and you can comment that out as well.

# Basic i18n 
# uncomment the line below to have minima
# use the data in _data/locales/[lang].yml 
#
# lang: fr

and hence wont explicitly need a en.yml

The second approach (preffered but not possible as of now) is to simply remove the default filters in the liquid tags for i18n strings:

<h1 class="page-heading">{{ site.data.locales[site.lang].posts }}</h1>

<p class="rss-subscribe">{{ site.data.locales[site.lang].subscribe }}
  <a href="{{ "/feed.xml" | relative_url }}">{{ site.data.locales[site.lang].via_RSS }}</a>
</p>

But the problem with this is Jekyll doesnt read data files from within the gem as of v3.3 so we cant ship default locale data alongwith the gem, and the user will have those strings missing from their site till he manually adds
So this way is closed for now.

@ashmaroli
Copy link
Member

Overall, this PR LGTM so far except for that small nit I mentioned above..
please proceed..

Also, please change to a more descriptive title.. (2nd try) is not required.
for example:
Add basic i18n support or similar

@oncleben31
Copy link
Contributor Author

Thank you for you very constructive comments. I will work on it next week.

@oncleben31 oncleben31 changed the title Basic i18n (2nd try) Basic i18n implementation Oct 7, 2016
@oncleben31
Copy link
Contributor Author

@ashmaroli any recommandation for the location to store the [lang].yml files ?
At the end the user will put it in the _data folder. But on a fresh install the folder will be empty and the user should find a message somewhere (documentation or default _config.ymlfile or README on Github, ....) explaining how to grab an existing one or create this file and where to put it.

My first opinion was on _data folder in minima theme folder, but you just explain me Jekyll currently can't access it. So it's perhaps not the best option.

@ashmaroli
Copy link
Member

ashmaroli commented Oct 7, 2016

One way is to store the data files in this repo itself.. like in your PR.. as example/_data/locales/fr.yml. Add a section in README informing Users wanting i18n to have to copy from that folder

@oncleben31
Copy link
Contributor Author

My PR is ready for review with strings and dates translation.

/cc @ashmaroli

{% assign date_format_to_be_translated = date_format_to_be_translated | replace: "%B", replacement %}
{% endif %}

{{ include.date | date: date_format_to_be_translated }}
Copy link
Member

Choose a reason for hiding this comment

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

Whoa! that's a LOT of assignments.. IMHO, there has to be a better way to have this implemented..

I advice you use comments to provide embedded documentation for this. Helps others who read your code..:

  • {% comment %}{% endcomment %} for liquid comments that do not get carried over to final site markup, and
  • <!-- --> for attribution (the source of this code, for example, with licence info, if available.) that does get carried over to site markup.

### Basic localisation (l10n)
# If you customize the lang parameter, minima will try to use the strings in _data/locales/[lang].yml
# Read https://github.com/jekyll/minima/README.md for more information.
#lang: fr
Copy link
Member

@ashmaroli ashmaroli Oct 12, 2016

Choose a reason for hiding this comment

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

I prefer comments to always have a whitespace after the #. Looks better.
(Plus, it doesnt seem like a reference-tag..
like how we would link to a heading in HTML e.g. /about.html#work-experience though I agree, its of no significance here.)


# Even the day and month names will be translated. You can customize your date by
# uncommenting the following line and customizing the date format:
#date_format: "Le %-d %B %Y"
Copy link
Member

@ashmaroli ashmaroli Oct 12, 2016

Choose a reason for hiding this comment

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

and here.. 😉

- oct.
- nov.
- déc.
full_month:
Copy link
Member

@ashmaroli ashmaroli Oct 12, 2016

Choose a reason for hiding this comment

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

why use lowercase names suddenly? If en.yml has Jan. | Feb., fr.yml should also be Jan. | Fev. etc

Copy link
Contributor Author

@oncleben31 oncleben31 Oct 12, 2016

Choose a reason for hiding this comment

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

FYI in french the standard format for dates use months in lowercase. Like "Mercredi 12 octobre 2016"

Copy link
Member

Choose a reason for hiding this comment

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

😃 Yes, I'd struck that comment out when I learnt about this convention..

@ashmaroli
Copy link
Member

Not a big fan of having an include with LOTS of assignments to implement date-translation..

  • we should have a built-in system to have that done in a cleaner and simpler way..

If, ThemeDataReader doesnt get merged upstream, and if a better alternative doesn't surface,
I'll release that monkey-patch as a separate gem..

@@ -51,6 +51,7 @@ The site's default CSS has now moved to a new place within the gem itself, [`ass
Minima theme allows a basic localisation of the texts used. To localise your site in your language:
1. Edit `_config.yml` to setup the `lang`parameter to your language.
2. copy an existing locales file ([from this repo](https://github.com/jekyll/minima/tree/master/example/_data/locales) or create a new one in `_data/locales/` named `[your lang parameter].yml`
3. (optional) change the `date_format` parameter in `_config.yml` by using the [date filter syntax](https://help.shopify.com/themes/liquid/filters/additional-filters#date). The day and month names will be translated.

--

Copy link
Member

Choose a reason for hiding this comment

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

Missed this.. you'll need some editing done here as well..
See how this section looks like in your repo.. ?

@oncleben31
Copy link
Contributor Author

@ashmaroli your comments have been taken into account.
I've succeeded to reduced the number of assign. I probably could do more but the code readability will be really low.

@ashmaroli
Copy link
Member

Thank you very much @oncleben31, I would still additionally add a new line between every {% endcomment %} and {% assign [..] %}.. but you may leave it as is for now..

👍 for the effort put in this PR..

/cc @jekyll/minima Any comments?

@DirtyF
Copy link
Member

DirtyF commented Jan 29, 2018

Closing this, as we don't intend to support i18n in this theme. Thanks for your effort @oncleben31

@DirtyF DirtyF closed this Jan 29, 2018
@jekyll jekyll locked and limited conversation to collaborators Apr 28, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants