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

Allow switching language using lang url param #219

Merged
merged 16 commits into from Apr 28, 2016
Merged

Allow switching language using lang url param #219

merged 16 commits into from Apr 28, 2016

Conversation

addshore
Copy link
Contributor

Currently if a language is selected and i18n messages
are missing the i18n code will be displayed instead

Currently if a language is selected and i18n messages
are missing the i18n code will be displayed instead

If no language is passed a default of de is used.
@jakobw
Copy link
Member

jakobw commented Apr 27, 2016

This approach seems a bit hacky to me. Can't we take the remaining strings from the index.html, put them in the i18n.json and then do the language switching entirely in JS?

@manicki
Copy link
Member

manicki commented Apr 27, 2016

I was actually also thinking about something like what @jakobw suggested. I am not sure which solution will be nicer though.

@addshore
Copy link
Contributor Author

Well assuming not all browsers have JS we would probably always went a landing page which also renders useful content without JS.
So if we want the landing page to also be localizable this would have to be done in PHP.

Fixing the landing page for non JS browsers is also something that should be done in my opinion saying that the site requires JS, right now it just half loads and doesn't work.

@manicki
Copy link
Member

manicki commented Apr 27, 2016

That's a good point. There should be some kind of i18n-enabled noscript info.

This branch is pushing towards the i18n not only
being handeled by JS thus it makes sense to split
the files out of the js dir.

For easy adding of more languages the i18n json
file has also been split into 1 per language.
@addshore
Copy link
Contributor Author

addshore commented Apr 27, 2016

I went ahead and moved the i18n files out of the JS directory (as they are used by the bit of php now)

I also went ahead and created a basic noscript landing page.

Current master looks like this:

image

Now it looks like this:

image

(although the message needs translating to German and adding to the i18n file)

@addshore
Copy link
Contributor Author

My current thoughts for the large blocks of text on the landing page are that they could be split out into separate html files which could then each be localized?

./i18n/html/de/about.html
./i18n/html/de/legalt.html

The text on feedback is probably small enough to keep in the json file but the about and legal sections just seems a bit insane.

After this the only js strings not in the i18n
files are the internal errors that afaik will
only go to the JS console and will not be
presented to users.
This makes the feedback form pass through
the language to the email backend so that
localized errors and thanks messages can be
send back to the frontend.
@addshore
Copy link
Contributor Author

As far as I can tell this PR now covers i18n for all strings (except internal exception messages which are not really user facing).

@jakobw
Copy link
Member

jakobw commented Apr 28, 2016

Ohh, that looks really cool now!

@jakobw
Copy link
Member

jakobw commented Apr 28, 2016

If I remember correctly I also suggested putting large blocks of texts in files so they can be translated but somehow after we discussed this we concluded that it would be better to have them all in one file (split large texts into paragraphs) to make it easier to just give it to people for translation. I think this looks pretty good though and it's super nice that we can even have a translated message for people who don't have JS activated. What do @manicki and @tobijat think? :)

if( !file_exists( $langFile ) ) {
$langFile = __DIR__ . '/../../../i18n/de/i18n.json';
}
$this->i18n = json_decode( file_get_contents( $langFile ), true );

if ( $this->requestValid( $app['request'] ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Could also be $this->requestValid( $request )

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

}

// Also replace html snippets
$htmlFiles = array(
Copy link
Member

Choose a reason for hiding this comment

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

We can haz new array [] syntax!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do everywhere! :)

@addshore
Copy link
Contributor Author

Yes the longer texts in files do seem quite manageable, however I feel that I should take a look at legal.html and remove the tracking switch from there....

I guess the switch itself should remain in the main html file and the strings around there should head to the i18n.json file


module.exports = new Polyglot( { phrases: i18n[ 'de' ] } );
var getParam = function getUrlParameter( sParam ) {
Copy link
Member

Choose a reason for hiding this comment

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

I thought JS would have a built-in thing to deal with URLs but google didn't spit anything out :/

This means the order of the replacements does not matter.
Before if the two keys below got replaced in this order bad
things would happen.

index.foo-bar
index.foo-bar-1
"index": {
"title": "Lizenzhinweisgenerator",
"description": "Lizenzhinweise für Bilder aus Wikipedia und Wikimedia Commons",
"no-script": "Sie müssen JavaScript zur Nutzung dieser Website aktiviert.",
Copy link
Member

Choose a reason for hiding this comment

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

I am guessing this is not entirely correct Kraut speak, although it contains all words I'd expect: "müssen", "Javascript", "aktiviert" :)
Let's have @jakobw and @WMDE-Fisch suggest something better

Copy link
Member

Choose a reason for hiding this comment

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

Haha, close enough though. Something like "Zur Nutzung dieser Webseite muss JavaScript aktiviert sein." would be better.

@manicki
Copy link
Member

manicki commented Apr 28, 2016

Looks really nice!
I really like having translatable noscript information and splitting i18n json files into separate file per language.
There is a little bit of inconsistency ie. when some i18n.index translations are missing there is no German fallback. For instance if I create an empty i18n/en/i18n.json file all HTML stuff will be in German same as all JS generated text but i18n.index things will remain untranslated, ie. it will be "{{i18n.index.title}}" instead of "Lizenz..." displayed on the front screen.
But I don't think it is a problem as it will only occur where there are some particular translations missing which can actually be helpful in the process of adding new language etc as it is obvious what is still not translated.

So basically it will be ready to go after updating one translation to what Jakob suggested.

@WMDE-Fisch
Copy link
Contributor

+:100:

@jakobw jakobw merged commit 61d3549 into master Apr 28, 2016
@jakobw jakobw deleted the langParam branch April 28, 2016 12:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants