Give page the right lang attribute

upsuper commented 1 year ago

Is your feature request related to a problem? Please describe.

Currently, the page always have <html lang="en"> regardless of what language the UI is using.

Describe the solution you'd like

Each language in resources/i18n should include an IETF BCP 47 language tag, for example, Japanese is ja, Simplified Chinese is generally zh-CN and Traditional Chinese is zh-TW.

When the UI language is updated, the lang attribute should be updated to the corresponding language tag.

Describe alternative solutions that would also satisfy this problem

Not aware of.

Additional context

This is a problem because language attribute affects

a11y, e.g. screen reader,
fallback font choice,
- which is especially a problem for CJK languages as they share Chinese characters, but different fonts have different character coverage, and the same character can come with different shape in different languages,
text transform, e.g. uppercasing,
- which is especially a problem for Latin-based languages, because different languages have different transformation rules, for example,
- in Turkish, the uppercase of "i" is "İ" rather than "I"
- in Irish, the uppercase of "gc" is "gC" not "GC"

(Though my main concern is on CJK.)

deluan commented 1 year ago

Yes, I think this can be done dynamically, as the <html lang="en"> tag comes from the static template in the server and the language is a property of the UI javascript client.

For reference: https://stackoverflow.com/a/52287911

subhajit20 commented 1 year ago

@deluan hey! can you tell me that is there any option in the application through which we can update the language of the software ? or the language will be updated organically ?

deluan commented 1 year ago

@subhajit20 Sorry, what language are you referring to? The lang property of the HTML as discussed above or the translation?

subhajit20 commented 1 year ago

Yes ! Do I have to populate all the language inside the lang attribute or anything else I am pretty confused 😕

deluan commented 1 year ago

Sorry again, but I'm not sure if you are trying to fix this issue or asking for support.

Assuming you want to fix the issue with a PR, you'd have to get the current language from locale key in localStorage and change the html lang attribute dynamically as per the SO link I posted above. This would probably have to happen in a useEffect callback in the main App.js AND when the user selects a new language in SelectLanguage.js file.

This of course assuming that changing the property dynamically actually solves the issues mentioned by @upsuper . If it does not, and we have to actually change the index.html template based on the selected language, the solution will be way more complex.

subhajit20 commented 1 year ago

@deluan yes this is what I wanted to know and yeas I am intend to solve the issue that's why I was asking.

If I get the language key from the local storage of the browser, i could add that keu inside the lang attribute right ?
Even if the user change the language then it will also reflect the lang key word and change that into a perticular language as user selects that right ?

deluan commented 1 year ago

Yes, that's how it should be implemented.
When the user selects a new language, the new value is stored in the localStorage (see the link to SelectLanguage.js above. In this case you would also need to update the lang attribute.

subhajit20 commented 1 year ago

@deluan okay now I get to know the entire scenario, thanks for making it understandable to me . I will try to fix this issue meanwhile if I need any help please keep in touch with me.

subhajit20 commented 1 year ago

@deluan hey is it okay ? SelectLanguage js - navidrome - Visual Studio Code 05-04-2023 11_15_03

subhajit20 commented 1 year ago

@deluan and see it also SelectLanguage js - navidrome - Visual Studio Code 05-04-2023 11_22_44

subhajit20 commented 1 year ago

@deluan hey have you check above image? it that the way how this should be implemented or anything else ?

deluan commented 1 year ago

Thanks for taking a look at this! The change in SelectLanguage.js is correct, but the one in the App.js is wrong, because it would only be called if the ActivityPanel is enabled, which may not always be the case, it is unrelated to the language selection. It would be better to put this in line 145 (the one I linked before), in the beginning of the AppWithHotkeys component. That's the entry point of the whole UI application.

As a side not, please do not post screenshots of code. The proper way to share it is using markdown code blocks. See https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#quoting-code

subhajit20 commented 1 year ago

okay sure I will fix that and sorry for uploading images as I am new to the world of open source.

subhajit20 commented 1 year ago

@deluan another thing is I have read the contribution guide line and I am little confused that should I create a branch name starts with the entire title/issue-number like this ? or anything else ? Give page the right lang attribute/2174

subhajit20 commented 1 year ago

@deluan in the line number 145 there is a if statement then do I have to put that line inside the if statement of before the if statement?

const AppWithHotkeys = () => {
  if (config.enableSharing && shareInfo) {
    let language = localStorage.getItem("locale");
    document.documentElement.lang = language;
    return <SharePlayer />
  }
  return (
    <HotKeys keyMap={keyMap}>
      <App />
    </HotKeys>
  )
}

deluan commented 1 year ago

I create a branch name starts with the entire title/issue-number like this ? or anything else ?

Check other PRs. There's no hard rule here, but you have to reference this issue, maybe just putting a #2174 in the PR's description.

in the line number 145 there is a if statement then do I have to put that line inside the if statement of before the if statement?

If you put it inside the if, it will only work if sharing is enabled, which does not make sense, right?

subhajit20 commented 1 year ago

@deluan hey I have made a pull request , kindly check that and let me know if any other modifications need to be made.

Thanks.

subhajit20 commented 1 year ago

@deluan hey whenever I am going to push the commit it shows this error right below


E:\Open Source Contribution\navidrome>git push origin Give_page_the_right_lang_attribute/2174
.git/hooks/pre-push: line 4: make: command not found
error: failed to push some refs to 'https://github.com/subhajit20/navidrome.git'

subhajit20 commented 1 year ago

@deluan hey I have made another fresh pr request. Can you check that once

deluan commented 1 year ago

Closed by #2299. @upsuper Can you confirm this works as intended?

upsuper commented 1 year ago

I checked Japanese and two Chinese ones, and I can confirm the font stack works as expected, so I would say it works.

(While testing, though, I noticed that Arabic doesn't switch the page to RTL, but this is probably a separate issue.)

github-actions[bot] commented 9 months ago

This issue has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

navidrome / navidrome