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

(fix) O3-4298: Capitalize language names in the change language modal #1254

Merged

Conversation

Muppasanipraneeth
Copy link
Contributor

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. Ensure your PR title includes a conventional commit label (such as feat, fix, or chore, among others). See existing PR titles for inspiration.

For changes to apps

If applicable

  • My work includes tests or is validated by existing tests.
  • I have updated the esm-framework mock to reflect any API changes I have made.

Summary

The "Change language" modal lists some language names in lowercase (e.g., "français," "español"), which is inconsistent with the proper capitalization of language names like "English" or "British English."

Screenshots

Before

Screen.Recording.2025-01-03.at.11.14.05.AM.mov

After

Screen.Recording.2025-01-03.at.11.43.21.AM.mov

Related Issue

Other

@Muppasanipraneeth Muppasanipraneeth force-pushed the fix/change-language-modal branch from cdfc659 to 02008a5 Compare January 3, 2025 06:56
@harshthakkr
Copy link

harshthakkr commented Jan 4, 2025

@Muppasanipraneeth British English should have 'E', not 'e'.
Screenshot 2025-01-04 at 11 14 26 AM

@Muppasanipraneeth
Copy link
Contributor Author

fixed British english to British English

Screen.Recording.2025-01-04.at.11.35.20.AM.mov

@@ -51,6 +51,9 @@ export default function ChangeLanguageModal({ close }: ChangeLanguageModalProps)
),
[allowedLocales],
);
const Capitalize = (str: string) => {
Copy link
Member

@denniskigen denniskigen Jan 6, 2025

Choose a reason for hiding this comment

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

If we must capitalize these language names, I think it'd be preferable to use the capitalize function from lodash-es here over something custom.

import { capitalize } from 'lodash-es';

@@ -69,7 +72,7 @@ export default function ChangeLanguageModal({ close }: ChangeLanguageModalProps)
key={`locale-option-${locale}-${i}`}
id={`locale-option-${locale}-${i}`}
name={locale}
labelText={languageNames[locale]}
labelText={Capitalize(languageNames[locale])}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
labelText={Capitalize(languageNames[locale])}
labelText={capitalize(languageNames[locale])}

@@ -34,10 +34,10 @@ describe(`Change Language Modal`, () => {
it('should correctly displays all allowed locales', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Strictly speaking, none of the strings in this file need to change to uppercase because the i flag in the regex will make the match case insensitive anyway. So we can safely undo the changes here.

@denniskigen
Copy link
Member

Also, I don;'t

British English should have 'E', not 'e'.

I don't think this is strictly necessary in this context. While technically "English" is a proper noun and should be capitalized in both British and American English, what matters most here is:

  • User comprehension
  • Consistency with the system's other language displays
  • Following the locale's own conventions for displaying language names

The Intl.DisplayNames API that's being used in the code (new Intl.DisplayNames([locale], { type: 'language' })) already follows the appropriate conventions for each locale. It returns the language name in the format that's standard for that locale, which is what we should use.

@NethmiRodrigo
Copy link
Collaborator

My only concern is how would this change affect non-english languages, where capitalization might not even be a concept?

@denniskigen denniskigen changed the title (fix) 03-4298 : change the language model to Capitalize (fix) 03-4298: Capitalize language names in the change language modal Jan 6, 2025
Copy link
Member

@mogoodrich mogoodrich left a comment

Choose a reason for hiding this comment

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

Seems like the right casing to me, thanks @denniskigen !

@denniskigen denniskigen force-pushed the fix/change-language-modal branch from bf601e2 to fc5529f Compare January 9, 2025 20:36
@denniskigen denniskigen changed the title (fix) 03-4298: Capitalize language names in the change language modal (fix) O3-4298: Capitalize language names in the change language modal Jan 9, 2025
Copy link
Member

@denniskigen denniskigen left a comment

Choose a reason for hiding this comment

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

@denniskigen denniskigen merged commit 97ed35e into openmrs:main Jan 9, 2025
14 checks passed
@Muppasanipraneeth
Copy link
Contributor Author

Thanks @denniskigen for Reviewing😀

Twiineenock pushed a commit to Twiineenock/openmrs-esm-core that referenced this pull request Jan 14, 2025
…al (openmrs#1254)

* fix : changed the language modal

* O3-4298 change the language modal

* modified the test

* rather than using capitalze from lodash-es done with function

* used capitalize from lodash-es

* updated the test

* Fixup

---------

Co-authored-by: Dennis Kigen <[email protected]>
@denniskigen denniskigen mentioned this pull request Jan 15, 2025
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants