-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Browser Language Preference Needs To Be Honored #17477
Conversation
…ng language decision
|
||
<%_ if (enableTranslation) { _%> | ||
findLanguage(): string { | ||
const languageKeys = Object.keys(JSON.parse(JSON.stringify(this.languages))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is JSON parse/stringify here, languages is already a proper object
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think this is right, with this logic you could end up with wrong versions for many languages, I'll rather do just the below
const currentLanguage = Object.keys(this.languages).includes(navigator.language) ? navigator.language : this.currentLanguage;
this.translationService().refreshTranslation(currentLanguage);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your feedback. I test your code and the outcome is the same for our test cases. I have changed the code in the branch last Friday. I don't know whether that is the right way to update a pull request or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Can you sign the CLA so that we can go further? If you do welcome! |
Is this one ready? |
@mraible It is, but only for Vue |
There's already many feature gap between frontend: if you agree let's not block single frontend tech feature |
@vw98075 Is this ready to be merged? If so, please mark it ready for review. |
@vw98075 It looks like the CLA hasn't been signed yet. That's why CI isn't succeeding. |
Fix #15744
Please make sure the below checklist is followed for Pull Requests.
When you are still working on the PR, consider converting it to Draft (below reviewers) and adding
skip-ci
label, you can still see CI build result at your branch.