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

Add change language option. #5693

Merged

Conversation

theSinner
Copy link
Contributor

Hi

I added the "choose language" option in the settings.
After choosing a language, the frontend side sets the language code in the cookie and reload the dashboard and the backend side checks the cookie before checking the "Accept-Language" header and load the dashboard based on the selected language.

@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 23, 2020
@k8s-ci-robot
Copy link
Contributor

Welcome @theSinner!

It looks like this is your first PR to kubernetes/dashboard 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/dashboard has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

@k8s-ci-robot k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Nov 23, 2020
@k8s-ci-robot k8s-ci-robot added language/fr Updates or issues for French translations. language/ja Updates or issues for Japanese translations. language/ko Updates or issues for Korean translations. language/zh Updates or issues for Chinese translations. labels Nov 23, 2020
@codecov
Copy link

codecov bot commented Nov 23, 2020

Codecov Report

Merging #5693 (758b5c7) into master (db5d615) will decrease coverage by 0.02%.
The diff coverage is 20.83%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5693      +/-   ##
==========================================
- Coverage   44.39%   44.36%   -0.03%     
==========================================
  Files         214      214              
  Lines        9119     9124       +5     
  Branches      113      113              
==========================================
  Hits         4048     4048              
- Misses       4797     4802       +5     
  Partials      274      274              
Impacted Files Coverage Δ
src/app/backend/handler/localehandler.go 61.01% <0.00%> (-3.27%) ⬇️
.../frontend/common/services/global/authentication.ts 37.14% <18.18%> (ø)
...c/app/frontend/common/services/global/namespace.ts 47.82% <40.00%> (-2.18%) ⬇️
src/app/frontend/index.config.ts 100.00% <100.00%> (ø)
...p/backend/integration/metric/common/aggregation.go 88.00% <0.00%> (-2.00%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update db5d615...758b5c7. Read the comment docs.

Copy link
Member

@floreks floreks left a comment

Choose a reason for hiding this comment

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

I like the overall idea. I'd propose a few changes:

  • Instead of keeping locales in the frontend, I'd move it to the backend and add a new endpoint to list supported locales. It can be a simple structure the same as the one used here.
  • I'd remove cookie reading logic in the backend and use angular interceptor to alter the Accept-Language request header similar to what we do when injecting the token.

@theSinner
Copy link
Contributor Author

theSinner commented Nov 23, 2020

  • Instead of keeping locales in the frontend, I'd move it to the backend and add a new endpoint to list supported locales. It can be a simple structure the same as the one used here.

Because Angular needs the locale list to generate the dashboard for each locale, I think it's better to keep the config together to make future changes easier.

  • I'd remove cookie reading logic in the backend and use angular interceptor to alter the Accept-Language request header similar to what we do when injecting the token.

It was my first idea to inject the "Accept-Language" header, and it's okay for the requests that angular send them, but the problem is when we want to load the dashboard itself. Because in this case, the browser sends the request with default parameters, so we can't inject the custom "Accept-Language" header. I had three choices to send the language parameter:

  • Query string
  • URL param
  • Cookie

Using query string and URL params cause more changes and I thought using cookies is a better idea.

@floreks
Copy link
Member

floreks commented Nov 24, 2020

Because Angular needs the locale list to generate the dashboard for each locale, I think it's better to keep the config together to make future changes easier.

That's a valid point that I can agree with. I'd then move it to the index.config.ts so that we have a single place where this is defined and we can inject it wherever we want.

As for the second point, it's true that the interceptor cannot be called before the app bundle is downloaded from the server. I did not think properly about the data flow here.

Using cookie then seems like the easiest approach as we have to save this information in there anyway.

@theSinner
Copy link
Contributor Author

Because Angular needs the locale list to generate the dashboard for each locale, I think it's better to keep the config together to make future changes easier.

That's a valid point that I can agree with. I'd then move it to the index.config.ts so that we have a single place where this is defined and we can inject it wherever we want.

As for the second point, it's true that the interceptor cannot be called before the app bundle is downloaded from the server. I did not think properly about the data flow here.

Using cookie then seems like the easiest approach as we have to save this information in there anyway.

Yes, you are right. Using "index.config" is a better idea. I didn't know this file exists.
I moved the languages list there as AVAILABLE_LANGUAGES variable and also make cookie value the most important value for choosing the language in the backend.

@helight
Copy link
Contributor

helight commented Nov 25, 2020

@theSinner you need to sign the cla(Contributor License Agreement )

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

@theSinner
Copy link
Contributor Author

@theSinner you need to sign the cla(Contributor License Agreement )

📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA.

Actually, I signed the cla. Maybe I should comment "I signed it" here to recheck.

@theSinner
Copy link
Contributor Author

I signed it

@floreks
Copy link
Member

floreks commented Nov 25, 2020

@theSinner your commits are missing user/email information. You need to configure git to use proper email when signing the commits.

@theSinner theSinner force-pushed the add-choose-language-to-local-settings branch 2 times, most recently from a82a249 to fbe2923 Compare November 25, 2020 09:33
@theSinner
Copy link
Contributor Author

@theSinner your commits are missing user/email information. You need to configure git to use proper email when signing the commits.

I fixed the name and email of the last commit, but the cla check still failed. Should I change the previous commits' email/name too?

@floreks
Copy link
Member

floreks commented Nov 25, 2020

Squash commits into a single commit, rebase and push force.

… Make lang value in cookie the most important, then the env variable and the least important is the default browser header.
@theSinner theSinner force-pushed the add-choose-language-to-local-settings branch from fbe2923 to 1469b1e Compare November 25, 2020 11:02
@k8s-ci-robot k8s-ci-robot removed the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Nov 25, 2020
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. labels Nov 25, 2020
@theSinner theSinner force-pushed the add-choose-language-to-local-settings branch from 02f7532 to 7135a91 Compare November 25, 2020 16:50
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Nov 25, 2020
@floreks
Copy link
Member

floreks commented Nov 26, 2020

@theSinner I have refactored code a bit and fixed the language caching issue where the browser would not reload language due to naming of output build files. I have also disabled language selector while in dev mode as it would not work anyway.

Thank you for your help and for creating the PR. I think it will really help Dashboard users.

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 26, 2020
@floreks
Copy link
Member

floreks commented Nov 26, 2020

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 26, 2020
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: floreks, theSinner

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@theSinner
Copy link
Contributor Author

@theSinner I have refactored code a bit and fixed the language caching issue where the browser would not reload language due to naming of output build files. I have also disabled language selector while in dev mode as it would not work anyway.

Thank you for your help and for creating the PR. I think it will really help Dashboard users.

Thank you for your time and help to fix the issues.
I'm glad I was able to contribute to Kubernetes and I will try to contribute more as I can.

@floreks floreks merged commit 72c275c into kubernetes:master Nov 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. language/de Updates or issues for German translations. language/fr Updates or issues for French translations. language/ja Updates or issues for Japanese translations. language/ko Updates or issues for Korean translations. language/zh Updates or issues for Chinese translations. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants