-
Notifications
You must be signed in to change notification settings - Fork 3
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
base: main
Are you sure you want to change the base?
Conversation
import { Select } from '@carbon/react'; | ||
import { SelectItem } from '@carbon/react'; |
There was a problem hiding this comment.
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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but
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'; |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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} /> |
There was a problem hiding this comment.
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
useEffect(() => { | ||
if (locale) { | ||
methods.setValue('locale', locale); | ||
} | ||
}, [locale]); |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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}`); |
There was a problem hiding this comment.
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.
Requirements
Summary
Issue: https://uwdigi.atlassian.net/browse/SSGSO-7
Screenshots
Screen.Recording.2024-11-05.at.04.19.44.mov
Related Issue
Other