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) Fix rules of hooks violations #272

Merged
merged 2 commits into from
May 16, 2024
Merged

(fix) Fix rules of hooks violations #272

merged 2 commits into from
May 16, 2024

Conversation

denniskigen
Copy link
Member

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

Fixes two violations of the rules of hooks as flagged by ESLint via the react-hooks rules of hooks rule.

Screenshots

CleanShot 2024-05-16 at 2  07 52@2x

Related Issue

Other


export const ProgramEnrollmentSubmissionAction: PostSubmissionAction = {
applyAction: async function ({ patient, encounters, sessionMode }, config) {
const encounter = encounters[0];
const encounterLocation = encounter.location['uuid'];
const { t } = useTranslation();
const t = window.i18next.t;
Copy link
Member Author

@denniskigen denniskigen May 15, 2024

Choose a reason for hiding this comment

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

@ibacher is there a better approach for getting the t function for use outside of React components? I don't remember. Also, I've not tested whether this works as expected.

Copy link
Member

Choose a reason for hiding this comment

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

If you add i18next as a peer dep, you should just be able to do:

import { t } from ‘i18next’;

Which is cleaner.

In normal components, we set the default namespace via a I18nextProvider. The raw t() function won’t pull from that context, so we’ll need to somehow set the namespace here for things to work as expected; otherwise it’ll always return a null object.

Copy link
Member Author

Choose a reason for hiding this comment

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

@vasharma05 how can I set the namespace here as Ian suggested?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @denniskigen!
You can use the translateFrom function present in the esm-framework for the translation.

how can I set the namespace here as Ian suggested?
The module name is the namespace for the respective app.

But please take care that you add a comment with `// t('key', 'default')
Thanks!

Copy link

github-actions bot commented May 15, 2024

Size Change: +55 B (0%)

Total Size: 1.12 MB

ℹ️ View Unchanged
Filename Size Change
dist/125.js 5.64 kB 0 B
dist/184.js 11.2 kB 0 B
dist/225.js 2.56 kB 0 B
dist/29.js 206 kB 0 B
dist/300.js 682 B 0 B
dist/327.js 1.72 kB +38 B (+2.26%)
dist/335.js 955 B 0 B
dist/353.js 3.02 kB 0 B
dist/368.js 241 kB +17 B (+0.01%)
dist/41.js 3.36 kB 0 B
dist/432.js 203 kB 0 B
dist/436.js 752 B 0 B
dist/505.js 6.94 kB 0 B
dist/540.js 2.63 kB 0 B
dist/55.js 2.14 kB 0 B
dist/635.js 14.2 kB 0 B
dist/66.js 78.5 kB 0 B
dist/760.js 5.25 kB 0 B
dist/797.js 18.7 kB 0 B
dist/942.js 482 B 0 B
dist/99.js 690 B 0 B
dist/993.js 3.08 kB 0 B
dist/main.js 309 kB 0 B
dist/openmrs-form-engine-lib.js 3.82 kB 0 B

compressed-size-action

Comment on lines 42 to 48
description: t(
'cannotEnrollPatientToProgram',
'This patient is already enrolled in the selected program',
),
Copy link
Member

@vasharma05 vasharma05 May 16, 2024

Choose a reason for hiding this comment

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

Suggested change
description: t(
'cannotEnrollPatientToProgram',
'This patient is already enrolled in the selected program',
),
// t( 'cannotEnrollPatientToProgram', 'This patient is already enrolled in the selected program')
description: translateFrom(
'@openmrs/esm-form-engine-lib',
'cannotEnrollPatientToProgram',
'This patient is already enrolled in the selected program',
),

Copy link
Member

Choose a reason for hiding this comment

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

Hhmmm curious whether that would work. We may have to add the translations to the form engine app and instead:

description: translateFrom(
              	 '@openmrs/esm-form-engine-app',
                'cannotEnrollPatientToProgram',
                'This patient is already enrolled in the selected program',
              )

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, @openmrs/esm-form-engine-lib wouldn't work without some additional hacking in the i18next "backend". We only support loading translations for things on the import map by default.

Copy link
Member

Choose a reason for hiding this comment

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

We may have to add the translations to the form engine app and instead:

Wait, why?

Copy link
Member

@samuelmale samuelmale May 16, 2024

Choose a reason for hiding this comment

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

Because it's not part of the import-map so at startup, the app-shell won't pickup the resource bundles

Copy link
Member

Choose a reason for hiding this comment

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

In short, if we have the translation present in the {lang}.json file, then whenever a form is loaded, the {lang}.json is loaded dynamically and hence all the translations in the form-engine-lib is present in the window.i18n, under the namespace @openmrs/form-engine-lib
Thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Yes you're right @vasharma05. The i18n setup in the form-engine reconciles with the global one so theoretically it should work.

Copy link
Member

Choose a reason for hiding this comment

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

That's right... I forgot about that.

Copy link
Member

Choose a reason for hiding this comment

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

@vasharma05 Well, yes, but translateFrom() is also basically equivalent to:

import { i18n: { t } } from 'i18next';

// ...

t('key', { ns: moduleName });

Which would be functionally identical.

Copy link
Member

Choose a reason for hiding this comment

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

Yes @ibacher, but when I tried this during the hackathon, it didn't work

The i18next was different from window.i18n, didn't get time to break down into it.

@samuelmale samuelmale merged commit 3bf2209 into main May 16, 2024
4 checks passed
@samuelmale samuelmale deleted the fix/rules-of-hooks branch May 16, 2024 18:57
vasharma05 pushed a commit that referenced this pull request May 27, 2024
* (fix) Fix rules of hooks violations

* Fixup translation function
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.

4 participants