-
Notifications
You must be signed in to change notification settings - Fork 533
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
Added translations in encounter page and minor fixes in patient registration form #10264
base: develop
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request introduces internationalization for patient gender display, enhances the patient registration form with a year of birth calculation feature, and improves the date input field's handling and layout. The changes focus on improving user experience by adding localization, providing more informative input feedback, and creating a more responsive design across different device screen sizes. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/ui/date-field.tsx (1)
Line range hint
45-101
: LGTM! Input length restrictions implemented correctly.The implementation properly limits input lengths for day (2 digits), month (2 digits), and year (4 digits) using slice(). The validation logic and focus management remain intact.
Consider adding leading zeros for single-digit day/month inputs for consistency:
- const newDay = e.target.value.slice(0, 2); + const newDay = e.target.value.slice(0, 2).padStart(2, '0');src/components/Patient/PatientInfoCard.tsx (1)
118-119
: Consider extracting the repeated gender display into a shared component.The implementation looks good, but the gender display logic is duplicated between mobile and desktop views.
Consider extracting it into a reusable component to follow DRY principles:
+ const GenderDisplay: React.FC<{ gender: string }> = ({ gender }) => { + const { t } = useTranslation(); + return ( + <> + {formatPatientAge(patient, true)} • {t(`GENDER__${gender}`)} + </> + ); + }; // In the render method: - {formatPatientAge(patient, true)} • {t(`GENDER__${patient.gender}`)} + <GenderDisplay gender={patient.gender} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/components/Patient/PatientInfoCard.tsx
(2 hunks)src/components/Patient/PatientRegistration.tsx
(2 hunks)src/components/ui/date-field.tsx
(4 hunks)src/components/ui/phone-input.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/ui/phone-input.tsx
🧰 Additional context used
📓 Learnings (1)
src/components/Patient/PatientRegistration.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#10260
File: src/components/Patient/PatientRegistration.tsx:286-289
Timestamp: 2025-01-28T15:50:07.442Z
Learning: For patient registration in the frontend, either year_of_birth or date_of_birth is required for successful registration. If date_of_birth is not available, year_of_birth must be present.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/components/ui/date-field.tsx (1)
104-104
: LGTM! Responsive layout improvements.The flex-wrap and adjusted gap sizes improve the component's responsiveness across different screen sizes.
src/components/Patient/PatientRegistration.tsx (2)
569-577
: LGTM! Year of birth calculation implemented correctly.The implementation aligns with the requirement that either year_of_birth or date_of_birth is needed for registration. The calculation and display are properly implemented.
Consider edge cases around new year transitions. For example, if someone is 25 years old in December 2024, they might have been born in either 1998 or 1999. Consider adding a note about this ambiguity.
718-718
: LGTM! Responsive grid layout improvements.The grid layout changes properly handle different screen sizes, switching from a single column on mobile to two columns on medium and larger screens.
src/components/Patient/PatientInfoCard.tsx (1)
104-105
: LGTM! Verify translation keys exist.The gender localization implementation looks good. However, let's verify that all possible gender values have corresponding translation keys.
✅ Verification successful
Translation keys verified and match gender values ✓
The gender values used in PatientInfoCard (
male
,female
,non_binary
,transgender
) have corresponding translation keys (GENDER__male
,GENDER__female
,GENDER__non_binary
,GENDER__transgender
) in the i18n files. The implementation is correct.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify translation keys for all possible gender values # Find all gender values used in the codebase echo "Searching for gender assignments in the codebase..." rg -g '*.{ts,tsx}' 'gender[^a-zA-Z].*=.*[^a-zA-Z]' --no-filename | sort -u # Find GENDER__ translation keys in i18n files echo -e "\nSearching for GENDER__ translation keys..." rg -g '*.{json,yaml,yml}' '"GENDER__.*"' --no-filename | sort -uLength of output: 1054
Script:
#!/bin/bash # Find Patient type/interface definition and PatientInfoCard usage echo "Searching for Patient type definition..." ast-grep --pattern 'interface Patient { $$$ gender: $_ $$$ }' echo -e "\nChecking gender values in PatientInfoCard..." rg -A 1 -B 1 'patient\.gender' src/components/Patient/PatientInfoCard.tsxLength of output: 1727
Script:
#!/bin/bash # Find GenderChoices type definition echo "Searching for GenderChoices type definition..." ast-grep --pattern 'type GenderChoices = $_' echo -e "\nSearching for enum or const definition related to gender choices..." ast-grep --pattern 'enum Gender$_ { $$$ }' echo -e "\nSearching for GENDER_TYPES constant..." rg -A 5 'GENDER_TYPES\s*='Length of output: 980
{form.getValues("age") && ( | ||
<div className="text-violet-600 text-sm font-bold"> | ||
{t("year_of_birth")}:{" "} | ||
{new Date().getFullYear() - |
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 think we can use differenceInYears
from date-fns
cc: @Jacobjeevan
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
src/components/ui/date-field.tsx
Outdated
const newDay = e.target.value; | ||
const newDay = e.target.value.slice(0, 2); |
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.
this is not necessary right? i don't see any cases where it'll have more than 2 characters
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.
@rithviknishad i used this to set limit for input length for fields DD MM YYYY,
Earlier we can enter any number of digits in the fields DD MM YYYY
Another approach would be adding maxLength for input for that i have to change input type to "text"
src/components/ui/date-field.tsx
Outdated
const newMonth = e.target.value; | ||
const newMonth = e.target.value.slice(0, 2); |
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.
here too
src/components/ui/date-field.tsx
Outdated
const newYear = e.target.value; | ||
const newYear = e.target.value.slice(0, 4); |
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.
this makes UX bad if they move their caret to left and start typing. numbers will start disappearing and carets start jumping here and there.
@rithviknishad i used .slice() to set limit for input length for fields DD MM YYYY, Another approach would be adding maxLength for input for that we need to change input type to "text" Anyway the form validation handles incorrect year Added max for year field |
…are_fe into typo-in-encounter
Proposed Changes
Fixes Fix typo in encounter, and some minor across platform #10233
Added missing typo in the encounter page (binary, symptoms and diagnosis)
Adjusted size of country select button
Responsive patient form and added limit to DD MM YYYY
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Improvements