-
-
Notifications
You must be signed in to change notification settings - Fork 4.3k
fix(settings): show active forced locale or language instead of 'No locale set' (#41543) #53364
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
base: master
Are you sure you want to change the base?
fix(settings): show active forced locale or language instead of 'No locale set' (#41543) #53364
Conversation
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.
Nice, thank you for the pull request! Added some comments :)
apps/settings/src/components/PersonalInfo/LanguageSection/__tests__/LanguageSection.spec.js
Outdated
Show resolved
Hide resolved
apps/settings/src/components/PersonalInfo/LanguageSection/__tests__/LanguageSection.spec.js
Outdated
Show resolved
Hide resolved
apps/settings/src/components/PersonalInfo/LanguageSection/__tests__/LanguageSection.spec.js
Outdated
Show resolved
Hide resolved
apps/settings/src/components/PersonalInfo/LanguageSection/__tests__/LanguageSection.spec.js
Outdated
Show resolved
Hide resolved
apps/settings/src/components/PersonalInfo/LanguageSection/__tests__/LanguageSection.spec.js
Outdated
Show resolved
Hide resolved
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.
Nice that there is a test but for this its probably also good to add a cypress e2e test.
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 haven't committed the cypress tests yet, as I'm struggling to get them running with my current setup (https://github.com/juliusknorr/nextcloud-docker-dev).
apps/settings/src/components/PersonalInfo/LocaleSection/__tests__/LocaleSection.spec.js
Outdated
Show resolved
Hide resolved
a852913
to
767d1d3
Compare
…ocale set' (fixes nextcloud#41543) Signed-off-by: Annabel Church <[email protected]>
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
767d1d3
to
015d87a
Compare
- Clarified the naming in the validate functions - Removed unnecessary mocks, and added stubs in tests. - Testing for existence of element with data-test attributes, and without relying on translated text. - Removed overly complex component mocking Signed-off-by: Annabel Church <[email protected]>
015d87a
to
262c9f8
Compare
I forgot to re-request a review, done now, @susnux thank you for your comments and labour |
No locale set
#41543Summary
When a forced locale or language is set by the administrator, this PR ensures the appropriate message is displayed instead of the misleading 'No locale set'.
Before
Issue #41543 - Screenshot
After
Forced locale / language:
User-set locale / language:
TODO
Notes
When the locale is set (either by the user or forced by an administrator), it is displayed in English, regardless of the current interface language. This matches existing behaviour in user settings.
Checklist