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

Fixed ui language detection #138

Closed

Conversation

serioustk
Copy link

Signed-off-by: Tobias Kleffmann [email protected]

Summary

Submit desired language as correct parameter ui=....
Removed obsolete code.

TODO

Checklist

  • Code is properly formatted
  • Sign-off message is added to all commits
  • Documentation (manuals or wiki) has been updated or is not required

Signed-off-by: Tobias Kleffmann <[email protected]>
@juliusknorr juliusknorr self-requested a review April 28, 2021 12:27
@juliusknorr juliusknorr added the 3. to review Waiting for reviews label Apr 28, 2021
Copy link
Member

@juliusknorr juliusknorr left a comment

Choose a reason for hiding this comment

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

Thanks a lot for your pull request, I have two small comments that we'd need to resolve, otherwise this makes lot of sense 👍

@@ -46,7 +46,7 @@ const getWopiUrl = ({ fileId, title, readOnly, closeButton, revisionHistory }) =
return Config.get('urlsrc')
+ 'WOPISrc=' + wopisrc
+ '&title=' + encodeURIComponent(title)
+ '&lang=' + languageToBCP47()
+ '&ui=' + getLocale().replace(/_/g, '-')
Copy link
Member

Choose a reason for hiding this comment

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

As https://wopi.readthedocs.io/en/latest/discovery.html#term-ui-llcc defines the UI language it should use getLanguage() in this case.

https://wopi.readthedocs.io/en/latest/faq/languages.html#languages
Single language codes from Nextcloud like "en" should also be fine here according to the Office API docs:

You may also use any value provided it is in the format described in RFC 1766.

@@ -20,35 +20,6 @@
*
*/

import { getLanguage, getLocale } from '@nextcloud/l10n'

const languageToBCP47 = () => {
Copy link
Member

Choose a reason for hiding this comment

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

The export in line 28 also needs to be removed to make the lint CI run happy.

@juliusknorr
Copy link
Member

After reading up on the documentation I came up with #148 which I tent to consider to be more aligned with how the language should be set on office online servers. Testing of that PR is also very welcome, we could still get this one in as a cleanup in order to not set the unused lang parameter. Thanks for getting this started anyways :)

@juliusknorr
Copy link
Member

Thanks a lot for getting this started, we've just merged a slightly different implementation of this in #148 on the backend side since the mapping to the correct language strings needed a bit more handling to properly work with office online.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews
Projects
None yet
Development

Successfully merging this pull request may close these issues.

russian language
2 participants