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

SSGSO-7: Configure PRO form language #3

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

reagan-meant
Copy link
Contributor

@reagan-meant reagan-meant commented Nov 4, 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

Issue: https://uwdigi.atlassian.net/browse/SSGSO-7

Screenshots

Screen.Recording.2024-11-05.at.04.19.44.mov

Related Issue

Other

Comment on lines +23 to +24
import { Select } from '@carbon/react';
import { SelectItem } from '@carbon/react';
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine these two imports. Also it's best to keep the library imports before the local file imports.

useLayoutType,
usePatient,
useSession,
} from '@openmrs/esm-framework';
import { type DefaultPatientWorkspaceProps } from '@openmrs/esm-patient-common-lib';
import { type SmsFormData } from './common/types';
import styles from './send-sms-form.scss';
import { saveQuestionnaire } from './common';
import SendSmsField from './send-sms-input.componet';
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit, but

Suggested change
import SendSmsField from './send-sms-input.componet';
import SendSmsField from './send-sms-input.component';

useLayoutType,
usePatient,
useSession,
} from '@openmrs/esm-framework';
import { type DefaultPatientWorkspaceProps } from '@openmrs/esm-patient-common-lib';
import { type SmsFormData } from './common/types';
import styles from './send-sms-form.scss';
Copy link
Contributor

Choose a reason for hiding this comment

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

Usually, style imports come last.

@@ -27,6 +36,7 @@ const SendSmsForm: React.FC<SendSmsFormProps> = ({
const isTablet = useLayoutType() === 'tablet';
const isOnline = useConnectivity();
const { patientUuid, patient } = usePatient();
const { locale, allowedLocales } = useSession();
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not entirely sure about this. What's the goal of locale switching here? Is the list of locales definitely the same as the list of locales for OpenMRS?

invalidText={methods.formState.errors.locale?.message}
{...methods.register('locale')}>
{allowedLocales?.map((allowedLocale) => (
<SelectItem key={allowedLocale} value={allowedLocale} text={allowedLocale} />
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably useful to look at how we get display names in the locale-switching component

Comment on lines +72 to +76
useEffect(() => {
if (locale) {
methods.setValue('locale', locale);
}
}, [locale]);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, it's probably better to switch the Select to a controlled input (one where we set the value) like this. This still results in default selecting the user's locale, but won't switch locales on this form if the user changes locales.

@@ -77,6 +94,7 @@ const SendSmsForm: React.FC<SendSmsFormProps> = ({
body: body,
source: source,
patientUuid: patientUuid,
locale: locale,
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few typos below this line that aren't part of this PR, but should be fixed.

const guid = uuid();
const body = window.location.host.concat(`/outcomes?pid=${guid}`);
const body = window.location.host.concat(`/outcomes?pid=${guid}&locale=${locale}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

Wherever possible, I tend to prefer to use a URLSearchParams object to construct arguments. This way the arguments you pass in the URL are always URL encoded, which ensures that they'll behave as you expect.

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