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

Added i18n support #262

Merged
merged 1 commit into from Nov 8, 2017
Merged

Added i18n support #262

merged 1 commit into from Nov 8, 2017

Conversation

eko
Copy link
Contributor

@eko eko commented Oct 13, 2017

Hello,

This is the pull request for the following issue: #26

Overview

I've added i18n support thanks to the following plugin: https://github.com/Anthony-Gaudino/jekyll-multiple-languages-plugin

Here is the features added in this PR:

  • Translates the homepage (/) and brings an english version of this page: /en,
  • Posts on homepage are filtered by current language,
  • A language switch between fr/en available in the header by clicking on a little flag,
  • Algolia search will return only the current language posts when searching in header (tested).

Additional information

This PR cannot be merged actually because one thing I find weird with it is that he needs to put all posts under i18n/{fr,en}/_posts subdirectory which is not cool so I've opened a PR to propose a parameter that will allow us to customized this directory and use: _posts/{fr,en}.

PR is opened here: kurtsson/jekyll-multiple-languages-plugin#107

Thank you

@ElevenWilson ElevenWilson temporarily deployed to i18n-support October 13, 2017 14:56 Inactive
@eleven-labs eleven-labs deleted a comment from ElevenWilson Oct 13, 2017
@@ -1,5 +1,11 @@
<!DOCTYPE html>
<html class="no-js">
Copy link
Contributor

Choose a reason for hiding this comment

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

Il faudrait changer l'attribut lang sur la balise <html> :

<!DOCTYPE html>
<html class="no-js" lang="{{ site.lang }}">
...
</html>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bonne remarque, merci @ulrik59, j'ai ajouté l'attribut :)

@eleven-labs eleven-labs deleted a comment from ElevenWilson Oct 14, 2017
@ElevenWilson ElevenWilson temporarily deployed to i18n-support October 14, 2017 10:47 Inactive
@eleven-labs eleven-labs deleted a comment from ElevenWilson Oct 14, 2017
@ElevenWilson ElevenWilson temporarily deployed to i18n-support October 14, 2017 10:56 Inactive
@ElevenWilson ElevenWilson temporarily deployed to i18n-support October 14, 2017 11:16 Inactive
@eleven-labs eleven-labs deleted a comment from ElevenWilson Oct 14, 2017
@eleven-labs eleven-labs deleted a comment from ElevenWilson Oct 14, 2017
@ElevenWilson ElevenWilson temporarily deployed to i18n-support October 14, 2017 11:24 Inactive
@eko eko force-pushed the i18n-support branch 2 times, most recently from 9a1bfbc to cd2e202 Compare October 28, 2017 10:21
@eko eko added RFR and removed WIP labels Oct 28, 2017
@eko eko changed the title [WIP] Added i18n support Added i18n support Oct 28, 2017
@eleven-labs eleven-labs deleted a comment from ElevenWilson Oct 28, 2017
@eleven-labs eleven-labs deleted a comment from ElevenWilson Oct 28, 2017
@eko eko added WIP and removed RFR labels Oct 28, 2017
@eleven-labs eleven-labs deleted a comment from ElevenWilson Oct 28, 2017
@@ -1,5 +1,8 @@
<div id="disqus_thread"></div>
<script type="text/javascript">
var disqus_config = function () {
this.language = window.site.lang;
Copy link
Contributor

@ulrik59 ulrik59 Nov 5, 2017

Choose a reason for hiding this comment

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

Ah j'avais pas vu ce fichier... Alors le souci ici, c'est que la variable window.site n'existe pas... Tu auras une erreur javascript...
Il faut soit bouger le js pour lancer disqus dans le fichier scripts.html soit il faut remplacer window.site.lang par '{{ site.lang }}' (je suis pour la seconde option)

Copy link
Contributor Author

@eko eko Nov 5, 2017

Choose a reason for hiding this comment

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

@ulrik59 Ça fonctionne pourtant parfaitement ainsi (dû au fait que le window.site.lang est appelé depuis une function).

Snippet pour reproduire :

function baz() {
    console.log('value: ' + window.foo);
}

window.foo = 'bar';
baz();

Copy link
Contributor

Choose a reason for hiding this comment

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

ça fonctionne mais c'est une mauvaise pratique et c'est dangereux. Rien ne dit que Disqus change son code et que la fonction s’exécute avant le parsing du code JS déclarant la variable. Si tu ne veux pas changer le fichier disqus.html, je serais plus confiant si la déclaration du window.site se faisait à la fin du <head/>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

La fonction de config est déclarée de notre côté donc Disqus peut changer ce qu'il voudra ça n'impactera pas notre déclaration ...

J'ai tout de même modifié pour mettre directement {{ site.lang }}, comme ça le débat du JS est clôt et on évite de revenir en arrière en remettant la déclaration du window.site dans le head.

@eko eko force-pushed the i18n-support branch 2 times, most recently from 8efc3bc to d8da3bb Compare November 5, 2017 16:37
@eleven-labs eleven-labs deleted a comment from ElevenWilson Nov 5, 2017
@ElevenWilson ElevenWilson temporarily deployed to i18n-support November 5, 2017 16:40 Inactive
@ElevenWilson ElevenWilson temporarily deployed to i18n-support November 5, 2017 16:41 Inactive
@ElevenWilson ElevenWilson temporarily deployed to i18n-support November 5, 2017 17:12 Inactive
@eleven-labs eleven-labs deleted a comment from ElevenWilson Nov 5, 2017
_algolia.yml Outdated
algolia:
application_id: '5IGTHBX5JS'
index_name: 'blog_eleven'
- index.html
Copy link
Contributor

Choose a reason for hiding this comment

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

le yaml n'est pas valide, je pense qu'il manque le exclude:

@eko
Copy link
Contributor Author

eko commented Nov 6, 2017

@VEBERArnaud Bien vu ! Merci, c'est corrigé

@ElevenWilson ElevenWilson temporarily deployed to i18n-support November 6, 2017 19:53 Inactive
@@ -11,7 +11,7 @@ <h3 class="read-also-title">A lire aussi</h3>
{% continue %}
Copy link
Contributor

Choose a reason for hiding this comment

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

Tu as oublié de traduire la balise <h3>A lire aussi</h3> au dessus.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Merci, j'ai ajouté la traduction

@@ -1,7 +1,7 @@
<footer class="slice newsletter">
<div class="container">
<a class="newsletter-link" target="_blank" href="http://eepurl.com/cOuOIf">
abonnez vous à notre newsletter !
{% translate global.newsletter_subscribe %}
</a>
</div>
</footer>
Copy link
Contributor

Choose a reason for hiding this comment

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

manque un retour à la ligne

@eleven-labs eleven-labs deleted a comment from ElevenWilson Nov 7, 2017
@ElevenWilson ElevenWilson temporarily deployed to i18n-support November 7, 2017 08:46 Inactive
@ElevenWilson ElevenWilson temporarily deployed to i18n-support November 8, 2017 13:52 Inactive
@eleven-labs eleven-labs deleted a comment from ElevenWilson Nov 8, 2017
@CaptainJojo
Copy link
Contributor

@eko le conflit et je valide :)

@eko
Copy link
Contributor Author

eko commented Nov 8, 2017

@CaptainJojo done :)

@CaptainJojo CaptainJojo merged commit 6377a29 into master Nov 8, 2017
@CaptainJojo CaptainJojo deleted the i18n-support branch November 8, 2017 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants