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

(chore) Transifex migration for translations to @openmrs/esm-form-engine-app #457

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

pirupius
Copy link
Member

@pirupius pirupius commented Jan 17, 2025

Requirements

  • This PR has a title that briefly describes the work done including the ticket number. If there is a ticket, make sure your PR title includes a conventional commit label. See existing PR titles for inspiration.
  • My work conforms to the OpenMRS 3.0 Styleguide and design documentation.
  • My work includes tests or is validated by existing tests.

Summary

As part of the migration to use Transifix and align missing translations from this repo

  • Migrated all translation files and deleted any translation setup files
  • Switched translation to use @openmrs/esm-form-engine-app namespace
  • Added translation provision for sidenav links

Screenshots

Sample screenshot in Arabic

Screenshot 2025-01-15 at 10 40 37

Related Issue

Other

@@ -98,7 +99,7 @@ function PageLink({ page, currentActivePage, pagesWithErrors, requestPage }: Pag
e.preventDefault();
requestPage(page.id);
}}>
<span>{page.label}</span>
<span>{t(page.label)}</span>
Copy link
Member Author

Choose a reason for hiding this comment

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

Adds support for translations to sidenav menu-items

Copy link

github-actions bot commented Jan 17, 2025

Size Change: -267 kB (-17.45%) 👏

Total Size: 1.27 MB

Filename Size Change
dist/219.js 0 B -264 kB (removed) 🏆
ℹ️ View Unchanged
Filename Size Change
dist/151.js 382 kB 0 B
dist/225.js 2.58 kB 0 B
dist/260.js 114 kB 0 B
dist/277.js 11.9 kB 0 B
dist/300.js 0 B -645 B (removed) 🏆
dist/335.js 0 B -968 B (removed) 🏆
dist/353.js 3.02 kB 0 B
dist/41.js 3.37 kB 0 B
dist/422.js 3.05 kB 0 B
dist/499.js 2.51 kB 0 B
dist/540.js 2.63 kB 0 B
dist/55.js 0 B -758 B (removed) 🏆
dist/606.js 2.24 kB 0 B
dist/635.js 14.4 kB 0 B
dist/658.js 1.86 kB +11 B (+0.6%)
dist/727.js 87.2 kB 0 B
dist/948.js 264 kB 0 B
dist/979.js 6.87 kB 0 B
dist/99.js 0 B -691 B (removed) 🏆
dist/993.js 3.09 kB 0 B
dist/main.js 357 kB -192 B (-0.05%)
dist/openmrs-esm-form-engine-lib.js 3.8 kB +2 B (+0.05%)

compressed-size-action

@pirupius pirupius changed the title (Chore) Transifix migration for translations to @openmrs/esm-form-engine-app (chore) Transifix migration for translations to @openmrs/esm-form-engine-app Jan 17, 2025
@pirupius pirupius mentioned this pull request Jan 17, 2025
3 tasks
@pirupius pirupius marked this pull request as ready for review January 17, 2025 06:59
@samuelmale
Copy link
Member

I hoped this was about setting up Transifex integration for this repo and not migrating the translation setup to the form-engine-app? Could I be missing something @pirupius?

@pirupius pirupius changed the title (chore) Transifix migration for translations to @openmrs/esm-form-engine-app (chore) Transifex migration for translations to @openmrs/esm-form-engine-app Jan 17, 2025
@pirupius
Copy link
Member Author

@samuelmale Initially the plan was to integrate Transifex but on second review and interaction with @ibacher, he pointed out that the convention for that integration is for apps and not libraries. That's the same convention that has been implemented for esm-core libraries as well as patient commons lib within the patient chart.
The focus then changed to moving the translations out of this library and using the @openmrs/esm-form-engine-app namespace where the translations will be managed.

@samuelmale
Copy link
Member

I’m concerned about the tight coupling between the “form-engine-app” and the library. This approach assumes that the form engine is only intended for use within the patient chart. As a result, internationalization (i18n) would break for those using the form engine outside the patient chart, or for users with a custom ESM designed for dispensing forms within the patient chart.

@pirupius
Copy link
Member Author

pirupius commented Jan 17, 2025

Maybe @vasharma05 and @ibacher can correct me on this;

Based on the setup and the way it has been implemented with other libraries, it isn't necessarily restricting the use of the translations to only the patient chart but it's just using the namespace @openmrs/esm-form-engine-app to pick the translations. Similarly to the way many custom esms have the esm-commons-lib as a dependency but the translations are picked from the esm-patient-chart.

This is an example with form builder with the local version of my form-engine-app linked with the translations added in this PR. You can see the Save and Cancel translations which were previously in the form-engine-lib are being picked automatically from the loaded version of the form-engine-app

The catch would be if an implementation is using the library but not using the form-engine-app which sounds unlikely.

Screenshot 2025-01-17 at 13 52 02

@pirupius pirupius requested a review from denniskigen January 17, 2025 14:03
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.

2 participants