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

Set html lang attribute from language setting #5685

Merged
merged 3 commits into from Nov 30, 2017

Conversation

@aidalgol
Copy link

aidalgol commented Nov 25, 2017

Fixes #5662. (Trivial two-line change.)

Signed-off-by: Aidan Gauland aidalgol@fastmail.net

@@ -1,5 +1,5 @@
<!doctype html>
<html lang="en" style="height: 100%;">
<html id="root" lang="en" style="height: 100%;">

This comment has been minimized.

Copy link
@t3chguy

t3chguy Nov 25, 2017

Collaborator

why adding a redundant id instead of using document.getElementsByTagName ?

This comment has been minimized.

Copy link
@aidalgol

aidalgol Nov 25, 2017

Author

No reason other than I thought that might be more explicit. I wasn't sure which would be more ideal. I can go ahead and change this to use document.getElementsByTagName instead if that is preferred.

This comment has been minimized.

Copy link
@turt2live

turt2live Nov 25, 2017

Member

There's a chance that home.html, when opened, contains extra html tags. To prevent conflicts, assigning an id is probably best. Although I'd prefix it to avoid more conflicts (eg mx_PageRoot).

@@ -382,6 +382,7 @@ async function loadLanguage() {
}
try {
await languageHandler.setLanguage(langs);
document.getElementById("root").setAttribute("lang", languageHandler.getCurrentLanguage());

This comment has been minimized.

Copy link
@MTRNord

MTRNord Nov 25, 2017

Contributor

This should propably be done in matrix-react-sdk inside the languageHandler.js. Otherwise it doesn't get changed if the user changes the language in the settings.

This comment has been minimized.

Copy link
@t3chguy

t3chguy Nov 25, 2017

Collaborator

that is true, and then even more evidently you can't rely on the outermost html element having an id since riot isn't the only consumer of matrix-react-sdk

This comment has been minimized.

Copy link
@pafcu

pafcu Nov 25, 2017

Contributor

You actually want document.documentElement.

This comment has been minimized.

Copy link
@pafcu

pafcu Nov 25, 2017

Contributor

@MTRNord Are you sure? Doesn't the entire page get reloaded when you change language, and, thus, loadLanguage() would be called again, updating the attribute?

This comment has been minimized.

Copy link
@aidalgol

aidalgol Nov 25, 2017

Author

@MTRNord I tested changing the language, and it did as @pafcu described: the app reloads and loadLanguage() is called again. If it is more correct to go into languageHandler.js, where in that module should it go?

This comment has been minimized.

Copy link
@MTRNord

MTRNord Nov 26, 2017

Contributor

@pafcu well the current would propably need a refactor as soon as the translation change uses states instead a full reload

This comment has been minimized.

Copy link
@pafcu

pafcu Nov 26, 2017

Contributor

@MTRNord And the "put it into the sdk" would need a refactor as soon as the split between the sdk and riot-web is made more clear.

Perhaps the correct approach is to fire a languageChanged event that the application can listen to. But considering how small the current proposed change is, I'd say use it as is, and change it if and when someone implements something more robust.

This comment has been minimized.

Copy link
@MTRNord

MTRNord Nov 26, 2017

Contributor

@pafcu what about putting this (in another PR) inside a promise returned from setLanguage()? That would propably make more sense to do

This comment has been minimized.

Copy link
@pafcu

pafcu Nov 26, 2017

Contributor

Does that solve the issue of an app-developer only wanting to change the language of the matrix-specifc part of their page, rather than the entire page?

This comment has been minimized.

Copy link
@MTRNord

MTRNord Nov 26, 2017

Contributor

@pafcu no but the issue that a reload might not happen in the future on language changes. (Thats what my point is)

Aidan Gauland
Change the root <html> element id to follow Matrix naming conventions.
@@ -382,6 +382,7 @@ async function loadLanguage() {
}
try {
await languageHandler.setLanguage(langs);
document.getElementById("mx_PageRoot").setAttribute("lang", languageHandler.getCurrentLanguage());

This comment has been minimized.

Copy link
@pafcu

pafcu Nov 25, 2017

Contributor

Instead of using a special id, you should just use document.documentElement. That way you don't need to add any id to the <html> element at all.

Use documentElement to get the root <html> element instead of adding an id to
the root element.
@aidalgol

This comment has been minimized.

Copy link
Author

aidalgol commented Nov 26, 2017

I can't make sense of the Travis CI failure, so I don't know what I broke.

@pafcu

This comment has been minimized.

Copy link
Contributor

pafcu commented Nov 26, 2017

@aidalgol I don't think you actually broke it. It looks more like a problem with Travis? Perhaps someone with admin access to this repository could try to re-run the tests.

@t3chguy

This comment has been minimized.

Copy link
Collaborator

t3chguy commented Nov 26, 2017

I restarted it but its still failing, I'll take a look at it now

@MTRNord

This comment has been minimized.

Copy link
Contributor

MTRNord commented Nov 26, 2017

@t3chguy looks like that some deps failed to install. Propably a timing issue :/ (and not related to this change)

Copy link
Member

ara4n left a comment

lgtm, thanks!

@ara4n ara4n merged commit 2a40298 into vector-im:develop Nov 30, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.