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-4366: Change languages in my account panel to sentence case #1277

Merged

Conversation

harshthakkr
Copy link
Contributor

@harshthakkr harshthakkr commented Jan 25, 2025

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

All language names in the “My Account” panel now follow the same sentence case format as seen in the “Change Language” modal, providing a consistent experience across the application:

Screenshot 2025-01-23 at 6 31 36 PM

Screenshots

Before

Screenshot 2025-01-23 at 6 25 13 PM Screenshot 2025-01-23 at 6 25 42 PM

After

Screenshot 2025-01-23 at 6 30 11 PM Screenshot 2025-01-23 at 6 30 36 PM

Related Issue

O3-4366

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.

LGTM. Thanks, @harshthakkr!

@harshthakkr
Copy link
Contributor Author

For some reason, these tests are failing:
Screenshot 2025-01-25 at 7 38 56 PM

@denniskigen
Copy link
Member

They seem OK here...

@harshthakkr
Copy link
Contributor Author

Just wanted to confirm, did you use this syntax: import capitalize from 'lodash-es/capitalize' in change-language.modal.tsx while running tests? @denniskigen

@denniskigen
Copy link
Member

I've not run the tests locally. What error are you seeing? It's not clear from the screenshot where the error is.

@harshthakkr
Copy link
Contributor Author

harshthakkr commented Jan 27, 2025

  1. Screenshot 2025-01-28 at 12 39 20 AM
  2. Screenshot 2025-01-28 at 12 41 15 AM
  3. Screenshot 2025-01-28 at 12 41 55 AM

A total of 4 errors in the same file

@denniskigen
Copy link
Member

I see. Add the following line:

'lodash-es/(.*)': 'lodash/$1' ,

to the primary-navigation app's jest config moduleNameMapper entry here.

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.

LGTM. Thanks, @harshthakkr!

@harshthakkr
Copy link
Contributor Author

Thank you, @denniskigen!

@denniskigen denniskigen merged commit f8f53c0 into openmrs:main Jan 27, 2025
14 checks passed
@harshthakkr harshthakkr deleted the fix/languages-in-my-account-panel branch January 27, 2025 19:37
@@ -5,6 +5,7 @@ module.exports = {
},
transformIgnorePatterns: [],
moduleNameMapper: {
'lodash-es/(.*)': 'lodash/$1',
Copy link
Member

Choose a reason for hiding this comment

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

I know this is post-commit, but the reason that this rule didn't exist is because we want you to use import { captialize } from 'lodash-es'.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

@denniskigen denniskigen Jan 28, 2025

Choose a reason for hiding this comment

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

I must've got this lopsided, sorry, @harshthakkr.

I thought we preferred import capitalize from 'lodash-es/capitalize' @ibacher

Copy link
Member

Choose a reason for hiding this comment

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

Fixed here

Copy link
Member

@ibacher ibacher Jan 29, 2025

Choose a reason for hiding this comment

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

With lodash, but not lodash-es that was the case. lodash-es is packaged as a ES Module whereas lodash is, I think, common-js, so importing import {capitalize} from 'lodash' ends up including all of lodash, but import {capitalize} from 'lodash-es' only imports the relevant parts of lodash-es—which is exactly what import capitalzie from 'lodash/capitalize' also accomplishes.

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.

3 participants