Patient header UI update in Patient details page #9377
Conversation
WalkthroughThis pull request encompasses updates to localization, UI components, and patient-related interfaces. The changes include modifying the English localization file with spelling corrections and new keys, updating the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
src/components/Patient/PatientHome.tsx (1)
279-309: Consider memoizing the skills formatting logicThe skills formatting and tooltip rendering logic could benefit from memoization to prevent unnecessary re-renders, especially for large skill sets.
+ const formattedSkills = useMemo(() => formatSkills(skillsQuery.data?.results), + [skillsQuery.data?.results]); - {!!skillsQuery.data?.results?.length && - formatSkills(skillsQuery.data?.results)} + {!!skillsQuery.data?.results?.length && formattedSkills}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Patient/PatientDetailsTab/Demography.tsx(1 hunks)src/components/Patient/PatientHome.tsx(1 hunks)
🔇 Additional comments (4)
src/components/Patient/PatientDetailsTab/Demography.tsx (1)
229-229: LGTM: Safe state value access implementation
The change to use state_object?.name with optional chaining is a safer way to access the state value and fixes the incorrect state value issue mentioned in the PR objectives.
src/components/Patient/PatientHome.tsx (2)
268-269: LGTM: Clean layout structure
The flex layout with proper gap spacing provides a responsive and well-organized structure for the patient header.
350-432: LGTM: Comprehensive status indicators
The status chips section effectively displays all relevant patient statuses with appropriate visual indicators and conditions.
public/locale/en.json (1)
1119-1119: LGTM: Improved text clarity
The text change from "Patient Details" to "Details of Patient" provides better clarity in the UI.
Mahendar0701
changed the title
|
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
Jacobjeevan
added
changes required
and removed
needs testing
needs review
merge conflict
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/ui/badge.tsx (1)
37-59: Consider adding aria-label for icon-only scenarios.The implementation looks good! For better accessibility, consider adding aria-label when the badge contains only icons without text.
- {startIcon && <CareIcon icon={startIcon} className="mr-1" />} + {startIcon && <CareIcon icon={startIcon} className="mr-1" aria-label={startIcon} />} <span>{props.children}</span> - {endIcon && <CareIcon icon={endIcon} className="ml-1" />} + {endIcon && <CareIcon icon={endIcon} className="ml-1" aria-label={endIcon} />}src/components/Patient/PatientHome.tsx (1)
568-577: Consider consolidating transfer status logic.The transfer status badge could be simplified using a single conditional render.
- {patientData.allow_transfer ? ( - <Badge variant="success" startIcon="l-unlock"> - {t("transfer_allowed")} - </Badge> - ) : ( - <Badge variant="warning" startIcon="l-lock"> - {t("transfer_blocked")} - </Badge> - )} + <Badge + variant={patientData.allow_transfer ? "success" : "warning"} + startIcon={patientData.allow_transfer ? "l-unlock" : "l-lock"} + > + {t(patientData.allow_transfer ? "transfer_allowed" : "transfer_blocked")} + </Badge>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Patient/PatientHome.tsx(5 hunks)src/components/ui/badge.tsx(3 hunks)
🔇 Additional comments (3)
src/components/ui/badge.tsx (1)
19-26: LGTM! Well-structured variant definitions.
The new variants (alert, danger, success) are properly defined with appropriate color schemes and hover states.
src/components/Patient/PatientHome.tsx (2)
291-434: LGTM! Well-structured patient header layout.
The new layout effectively organizes patient information with clear sections for:
- Facility details
- Treating doctor with skills tooltip
- Assigned doctor with WhatsApp integration
- Assigned volunteer
- Status badges with appropriate visual indicators
347-355:
Security: Enhance WhatsApp link sanitization.
The current phone number sanitization can be improved for better security.
This was previously flagged in an earlier review. Please implement the suggested phone number sanitization function.
|
@Jacobjeevan i replaced chip with badge . can you once look into it |
…r0701/care_fe into patient-header-ui-design
…r0701/care_fe into patient-header-ui-design
|
👋 Hi, @Mahendar0701, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@Mahendar0701 clear the merge conflict |
Resolved |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Patient/PatientHome.tsx (2)
358-366: Initial visit badge usage is consistent.
Using a custom variant for the badge is clear. Possibly consider reusing a standard variant for uniform styling, but not critical.
405-415: Telemedicine and allergy indicators.
Multiple badges in a single row can get cluttered but is still acceptable. The approach is consistent with the rest of the code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
public/locale/en.json(1 hunks)src/components/Common/AuthorizedButton.tsx(2 hunks)src/components/Patient/PatientDetailsTab/Demography.tsx(2 hunks)src/components/Patient/PatientHome.tsx(14 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/components/Patient/PatientDetailsTab/Demography.tsx
- public/locale/en.json
🔇 Additional comments (22)
src/components/Common/AuthorizedButton.tsx (3)
7-8: Switching to a local Button component is consistent with the app’s design.
The import statement references a local “Button” component instead of a headless UI component, aligning with the unified button styling approach seen across the application.
9-12: Clear and concise default prop usage.
Using a default authorizeFor prop returning true ensures the button is always enabled unless explicitly restricted. This approach keeps the API consistent. No issues found.
23-24: Default export of the component.
Exporting the component as the default export is valid, simplifying imports in other files. No issues found.
src/components/Patient/PatientHome.tsx (19)
9-9: Consistent import for authorized UI elements.
Importing AuthorizedButton from a central location helps maintain consistency. No issues found.
42-42: Introducing Badge for state indicators.
Pulling in the Badge component is aligned with the shift away from chips. No issues found.
210-210: Clear textual representation of patient data.
Displaying patient age, gender, and blood group is straightforward. The usage of fallback " - " for absent blood group is helpful for clarity.
275-285: Facility details block looks good.
Graceful fallback to “-” ensures the UI doesn’t break if facility details are missing. Implementation is fine.
286-316: Properly displaying treating physician details.
The tooltip-based approach for revealing additional physician skills is a good UX choice. Code structure is clear, and the fallback logic is handled gracefully.
318-344: Repeating phone sanitization suggestion.
Consider sanitizing the phone number before appending it to the WhatsApp URL to prevent potential injection or formatting issues.
345-353: Assigned volunteer section is concise and functional.
Straightforward display logic with fallback. No issues found.
367-375: Vaccination badge appropriately highlighted.
The icon plus text approach is a clear representation. Implementation is straightforward.
377-404: Antenatal and postpartum indicators.
Using separate badges for antenatal/postpartum states is logical. The color scheme with pink variants for these statuses is readable. No issues found.
416-422: Location-based badge.
Displaying district and state in a single badge is effective. Implementation is straightforward and uses fallback strings earlier.
Line range hint 429-440: Tab navigation logic is clear.
Conditionally adding an active border for the selected tab helps indicate the current route. Accessibility attributes (aria-selected, role="tab") are also well-used.
470-475: Investigation summary button.
Users can easily navigate to the relevant reports. Good use of an icon for clarity.
Line range hint 491-517: Patient files and volunteer assignment actions.
Volunteer assignment logic is guarded behind authorization checks, which is good for security. Implementation looks sound.
Line range hint 532-570: Transfer toggling mechanism.
The handlePatientTransfer function updates the allow_transfer field properly. This is a good pattern for controlling patient record updates.
560-572: Transfer status badges.
Using a primary badge for allowed and a warning badge for blocked is an effective visual distinction. Logic is straightforward.
604-604: Last discharge reason.
Displays a default dash when no discharge reason is found. Implementation is correct.
638-638: Last updated by section.
Tooltips for both relative and absolute timestamps are helpful. No issues found.
659-659: Created by information.
Similar pattern to “last updated by,” consistent from a UI standpoint.
688-688: Death report generation button.
Provides a clear action for generating a death report if the patient has an “Expired” discharge reason. Straightforward approach.
| const AuthorizedButton: React.FC<AuthorizedElementProps & ButtonProps> = ({ | ||
| authorizeFor = () => true, | ||
| ...props | ||
| }) => { |
There was a problem hiding this comment.
let's drop the default. If we are going to use AuthorizedButton, then let's mandate the prop. If we are not going to pass the authorizeFor prop, we should be using the normal Button itself.
| const AuthorizedButton: React.FC<AuthorizedElementProps & ButtonProps> = ({ | |
| authorizeFor = () => true, | |
| ...props | |
| }) => { | |
| const AuthorizedButton: React.FC<AuthorizedElementProps & ButtonProps> = (props) => { |
There was a problem hiding this comment.
@rithviknishad can you once check
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Common/AuthorizedButton.tsx (1)
13-17: Consider enhancing prop validationWhile the authorizeFor validation is good, consider adding type checking to ensure the prop is of the correct type when provided.
if (!authorizeFor) { throw new Error( "The 'authorizeFor' prop is required for AuthorizedButton.", ); } +if (typeof authorizeFor !== 'string' && !Array.isArray(authorizeFor)) { + throw new Error( + "The 'authorizeFor' prop must be a string or an array of strings.", + ); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/AuthorizedButton.tsx(2 hunks)
🔇 Additional comments (4)
src/components/Common/AuthorizedButton.tsx (4)
9-12: LGTM! Clean component declaration with proper type definitions
The component declaration properly combines both ButtonProps and AuthorizedElementProps types, ensuring proper type checking for all props.
Line range hint 18-27: LGTM! Clean implementation with proper prop handling
The component correctly handles authorization logic and properly combines the disabled state with authorization status.
29-29: LGTM! Clean default export
The default export aligns with modern React practices and provides flexibility in importing.
5-5: Verify the Button component styling compatibility
The Button component import has been changed from headlessui to a local UI component. Please ensure that this change maintains consistent styling and behavior across all usage locations.
✅ Verification successful
Let me gather more information about the Button component usage to better understand the impact.
Let me check one more thing to understand the Button component's implementation and its compatibility.
Button component change is compatible and safe
The change from headlessui to the local UI Button component is safe and maintains consistent styling and behavior because:
- The local Button component is a well-structured component with proper styling variants and size options
- The AuthorizedButton correctly forwards all ButtonProps and handles the disabled state appropriately
- The Button component is widely used across the codebase (20+ files) as the standard button implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Find all files that use AuthorizedButton to verify visual consistency
rg -l "AuthorizedButton" | xargs rg -l "Button.*disabled.*isAuthorized"
Length of output: 114
Script:
#!/bin/bash
# Check the implementation of the local Button component
ast-grep --pattern 'export function Button($$$) { $$$ }' src/components/ui/button.tsx
# Find all usages of AuthorizedButton to understand its implementation
ast-grep --pattern 'const AuthorizedButton = $$$' src/components/Common/AuthorizedButton.tsx
# Find other Button usages in the codebase for consistency
rg "from.*components/ui/button" -A 2
Length of output: 5135
Script:
#!/bin/bash
# Check the Button component's implementation
cat src/components/ui/button.tsx
# Check the AuthorizedButton implementation to see how it uses the Button component
cat src/components/Common/AuthorizedButton.tsx
Length of output: 3398
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Common/AuthorizedButton.tsx (2)
Line range hint
13-23: Consider extracting button-specific propsWhile the implementation is clean, spreading all props to the Button component might pass unnecessary props. Consider extracting only ButtonProps:
-const AuthorizedButton: React.FC<AuthorizedButtonProps> = (props) => { +const AuthorizedButton: React.FC<AuthorizedButtonProps> = ({ authorizeFor, children, ...buttonProps }) => { return ( - <AuthorizedChild authorizeFor={props.authorizeFor}> + <AuthorizedChild authorizeFor={authorizeFor}> {({ isAuthorized }) => ( - <Button {...props} disabled={props.disabled || !isAuthorized}> - {props.children} + <Button {...buttonProps} disabled={buttonProps.disabled || !isAuthorized}> + {children} </Button> )} </AuthorizedChild> ); };
25-25: Consider using named exportWhile default exports work, named exports are generally preferred for better maintainability and refactoring support. However, if this follows your project's conventions, feel free to keep it as is.
-export default AuthorizedButton; +export { AuthorizedButton };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/AuthorizedButton.tsx(2 hunks)
🔇 Additional comments (2)
src/components/Common/AuthorizedButton.tsx (2)
1-7: LGTM! Clean import organization
The imports are well-structured using absolute paths and properly organized by separating external (React) from internal dependencies.
9-11: LGTM! Well-structured type definition
The AuthorizedButtonProps type effectively combines the necessary props while making authorizeFor mandatory, addressing the previous review feedback.
Proposed Changes
Fixes Update Patient Header and state value in Patient details page #9376
Updated patient Header
Updated state value in demography tab
Updated tranfer labels to relevant colours
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Style