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

(feat) Setup i18n for form engine lib #215

Merged
merged 8 commits into from
Apr 24, 2024

Conversation

vasharma05
Copy link
Member

@vasharma05 vasharma05 commented Apr 24, 2024

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

This PR implements internationalization in the Form Engine Library. Whenever the form engine lib is loaded, the lib will look for the active locale via the window.i18next.language and then load the particular file from the translation folder into the i18next instance. The same instance is then shared with the OHRIForm using the I18NextProvider via a HOC withTranslation.

Please note that in case the language is loaded, the corresponding translation resource will be loaded into the i18next in the runtime.

Screenshots

image image

Related Issue

TBD

Other

@vasharma05 vasharma05 force-pushed the feat/form-engine-translations branch from 271041b to e2f7ab8 Compare April 24, 2024 15:23
@vasharma05 vasharma05 force-pushed the feat/form-engine-translations branch 2 times, most recently from 3f0d89f to 0c249a7 Compare April 24, 2024 15:32
Copy link

github-actions bot commented Apr 24, 2024

Size Change: -332 kB (-21%) 🎉

Total Size: 1.22 MB

Filename Size Change
dist/154.js 0 B -80.8 kB (removed) 🏆
dist/29.js 206 kB -14.3 kB (-6%)
dist/311.js 0 B -237 kB (removed) 🏆
ℹ️ View Unchanged
Filename Size Change
dist/100.js 2.41 kB 0 B
dist/17.js 16.4 kB 0 B
dist/224.js 237 kB 0 B
dist/225.js 2.57 kB 0 B
dist/248.js 108 kB 0 B
dist/300.js 592 B 0 B
dist/327.js 1.58 kB 0 B
dist/353.js 3.02 kB 0 B
dist/459.js 6.07 kB 0 B
dist/505.js 7.46 kB 0 B
dist/540.js 2.63 kB 0 B
dist/561.js 487 B 0 B
dist/599.js 2.51 kB 0 B
dist/606.js 2.23 kB 0 B
dist/631.js 80.8 kB 0 B
dist/633.js 95 kB 0 B
dist/635.js 14.2 kB 0 B
dist/791.js 9.8 kB 0 B
dist/828.js 94 kB 0 B
dist/886.js 6.94 kB 0 B
dist/942.js 487 B 0 B
dist/993.js 3.08 kB 0 B
dist/main.js 312 kB +299 B (0%)
dist/openmrs-form-engine-lib.js 3.82 kB +50 B (+1%)

compressed-size-action

src/i18n.ts Outdated
@@ -0,0 +1,23 @@
function loadResourcesFromFile() {
const lang = window.i18next.language;
Copy link
Member

Choose a reason for hiding this comment

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

Rather than directly referencing window.i18next, I'd mark i18next as a peer dependency and import it in this file.

Copy link
Member Author

Choose a reason for hiding this comment

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

Getting undefined with i18next reference. Do you know what might be the reason?

Copy link
Member

Choose a reason for hiding this comment

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

When in the app is that happening? It's vaguely possible that language is undefined, but window.i18next should be set here. That's called after apps are registered, but it should be before anything gets mounted (since single-spa isn't called until setupI18n() succeeds).

Copy link
Member Author

Choose a reason for hiding this comment

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

image image

src/i18n.ts Outdated Show resolved Hide resolved
src/i18n.ts Outdated Show resolved Hide resolved
src/i18n.ts Outdated Show resolved Hide resolved
src/withTranslation.tsx Outdated Show resolved Hide resolved
translations/fr.json Outdated Show resolved Hide resolved
src/withTranslation.tsx Outdated Show resolved Hide resolved
tsconfig.json Outdated Show resolved Hide resolved
@denniskigen
Copy link
Member

window.18next.on is breaking the tests. Do we have a mock for it somewhere you're aware of, @ibacher?

@ibacher
Copy link
Member

ibacher commented Apr 24, 2024

@denniskigen No, but I think my suggestion of using i18next as a shared library will render it unnecessary. Turns out there's a mock for that in the framework mock!

@vasharma05
Copy link
Member Author

Hi @ibacher!
I have added another set of changes, please review.

image

Comment on lines 4 to 13
export default function withFormEngineTranslations(WrappedComponent: React.ComponentType<any>) {
return function WithTranslation(props: any) {
return (
<I18nextProvider i18n={window.i18next} defaultNS="@openmrs/openmrs-form-engine-lib">
<WrappedComponent {...props} />
</I18nextProvider>
);
};
}
Copy link
Member

@denniskigen denniskigen Apr 24, 2024

Choose a reason for hiding this comment

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

@vasharma05 There's a bunch of type errors in this file

Copy link
Member Author

Choose a reason for hiding this comment

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

Should be resolved now, add the mock for I18nextProvider

denniskigen

This comment was marked as duplicate.

denniskigen

This comment was marked as duplicate.

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.

Really excellent work, @vasharma05!

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

I wonder if some of the issue is that we're using i18next 21.x rather than 23.x? Anyways, very nice work!

@denniskigen denniskigen merged commit 4f50f9d into main Apr 24, 2024
4 checks passed
@denniskigen denniskigen deleted the feat/form-engine-translations branch April 24, 2024 20:11
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