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.

Sign up for GitHub

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

Jump to bottom

Set html lang attribute from language setting #5685

Merged
merged 3 commits into from
Nov 30, 2017
Merged

Set html lang attribute from language setting #5685

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0a5c28
Set html lang attribute from language setting
aidalgol Nov 25, 2017
43b3c01
Change html root id
aidalgol Nov 25, 2017
c4dc59d
Use documentElement instead of getElementById
aidalgol Nov 26, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/vector/index.html
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<!doctype html>
<html lang="en" style="height: 100%;">
<html id="root" lang="en" style="height: 100%;">
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

<head>
<meta charset="utf-8">
<title>Riot</title>
Expand Down
1 change: 1 addition & 0 deletions src/vector/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ async function loadLanguage() {
}
try {
await languageHandler.setLanguage(langs);
document.getElementById("root").setAttribute("lang", languageHandler.getCurrentLanguage());
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

You actually want document.documentElement.

Copy link
Contributor

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@pafcu pafcu Nov 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

} catch (e) {
console.error("Unable to set language", e);
}
Expand Down