Fix: Distinguish Active and Ended Medicines #10963
Conversation
WalkthroughThis change updates the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant MRQ as MedicationRequestQuestion Component
participant S as Sorting Logic
participant B as Badge Component
U ->> MRQ: Initiate component render
MRQ ->> S: Sort medications by status
S -->> MRQ: Return sortedMedications
MRQ ->> B: Conditionally render badge based on status
B -->> MRQ: Return styled badge
MRQ -->> U: Display sorted list with badges
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 (1)
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. |
|
@nihal467 @amjithtitus09 Review the PR Please. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (4)
536-550: Consider refactoring the nested conditional rendering.The current implementation has nested conditional checks which can make the code harder to maintain. Consider simplifying the logic.
- <span> - {(medication.status === "active" || - medication.status === "ended") && ( - <Badge - variant={medication.status === "active" ? "primary" : "outline"} - className={`inline-flex items-center gap-1 ${ - medication.status === "ended" - ? "bg-gray-50/50 text-gray-500 border-gray-200" - : "" - }`} - > - {t(`${medication.status}`)} - </Badge> - )} - </span> + {(medication.status === "active" || medication.status === "ended") && ( + <Badge + variant={medication.status === "active" ? "primary" : "outline"} + className={`inline-flex items-center gap-1 ${ + medication.status === "ended" + ? "bg-gray-50/50 text-gray-500 border-gray-200" + : "" + }`} + > + {t(`${medication.status}`)} + </Badge> + )}
186-191: Consider adding type safety for medication status.The current implementation relies on string comparisons for status values without type validation. Consider using a more type-safe approach.
+ // Define allowed status types + type MedicationStatus = 'active' | 'ended' | string; + const sortedMedications = [...medications].sort((a, b) => { - const statusOrder = { active: 0, ended: 1 }; - const aOrder = statusOrder[a.status as keyof typeof statusOrder] ?? 2; - const bOrder = statusOrder[b.status as keyof typeof statusOrder] ?? 2; + const statusOrder: Record<MedicationStatus, number> = { active: 0, ended: 1 }; + const aOrder = statusOrder[a.status as MedicationStatus] ?? 2; + const bOrder = statusOrder[b.status as MedicationStatus] ?? 2; return aOrder - bOrder; });
537-547: Extract status badge into a separate component for reusability.Since the status badge might be used elsewhere in the application, consider extracting it into a reusable component.
This would make the code more maintainable and promote consistency across the application when displaying medication statuses.
// Suggested component (to be added elsewhere) interface MedicationStatusBadgeProps { status: string; } const MedicationStatusBadge: React.FC<MedicationStatusBadgeProps> = ({ status }) => { if (status !== 'active' && status !== 'ended') return null; return ( <Badge variant={status === "active" ? "primary" : "outline"} className={`inline-flex items-center gap-1 ${ status === "ended" ? "bg-gray-50/50 text-gray-500 border-gray-200" : "" }`} > {t(`${status}`)} </Badge> ); }; // Then in your render function: // <MedicationStatusBadge status={medication.status} />
186-191: Add caching for sorted medications to improve performance.The current implementation re-sorts the medications array on every render. Consider using memoization to improve performance if the medications array is large.
- const sortedMedications = [...medications].sort((a, b) => { - const statusOrder = { active: 0, ended: 1 }; - const aOrder = statusOrder[a.status as keyof typeof statusOrder] ?? 2; - const bOrder = statusOrder[b.status as keyof typeof statusOrder] ?? 2; - return aOrder - bOrder; - }); + const sortedMedications = React.useMemo(() => { + return [...medications].sort((a, b) => { + const statusOrder = { active: 0, ended: 1 }; + const aOrder = statusOrder[a.status as keyof typeof statusOrder] ?? 2; + const bOrder = statusOrder[b.status as keyof typeof statusOrder] ?? 2; + return aOrder - bOrder; + }); + }, [medications]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (4)
18-18: Added necessary UI component import.The Badge component import is now included to support the new visual indicators for medication status.
186-191: Good implementation of medication sorting logic.The sorting logic effectively prioritizes medications with "active" status (order 0) before those with "ended" status (order 1), and places all other statuses (order 2) at the end. This meets the PR objective of distinguishing between active and ended medicines.
The implementation uses a clean approach with a statusOrder object and proper TypeScript typing with
keyof typeof statusOrder.
273-273: Successfully applied the sorting to the rendered medication list.The change from
medications.maptosortedMedications.mapensures that medications are displayed in the prioritized order (active first, then ended, then others).
526-550: Well-implemented visual distinction between active and ended medications.The code effectively implements the PR objective by:
- Adding status badges that visually distinguish between active and ended medications
- Using appropriate styling (primary badge for active, outline with gray for ended)
- Adding conditional text styling (graying out ended medication names)
- Ensuring the UI changes only apply to medications with relevant statuses
The implementation includes proper class conditionals and follows the existing styling patterns of the application.
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx
Outdated
Show resolved
Hide resolved
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx
Outdated
Show resolved
Hide resolved
…estion.tsx Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx
Outdated
Show resolved
Hide resolved
|
PR's not following the merge checklist will not be considered for review |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (1)
544-546: 🛠️ Refactor suggestionEnsure translation key format is correct
The current translation usage might not work as expected. Using template literals with the t function can be problematic if the translation system isn't configured to handle dynamic keys.
- {t(`${medication.status}`)} + {t(medication.status)}Also, ensure that both "active" and "ended" translations exist in all locales, not just English.
🧹 Nitpick comments (1)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (1)
186-191: Good implementation of medication sorting logicThe sorting implementation correctly prioritizes "active" medications over "ended" ones, improving the user experience. The use of
keyof typeoffor type safety is a good practice. Consider usinguseMemofor performance optimization if there are many medications:- const sortedMedications = [...medications].sort((a, b) => { + const sortedMedications = React.useMemo(() => [...medications].sort((a, b) => { const statusOrder = { active: 0, ended: 1 }; const aOrder = statusOrder[a.status as keyof typeof statusOrder] ?? 2; const bOrder = statusOrder[b.status as keyof typeof statusOrder] ?? 2; return aOrder - bOrder; - }); + }), [medications]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx(4 hunks)
🔇 Additional comments (3)
src/components/Questionnaire/QuestionTypes/MedicationRequestQuestion.tsx (3)
18-18: Appropriate use of Badge componentGood addition of the Badge component import which is used to visually distinguish medication statuses.
273-273: LGTM! Using the sorted medications arrayAppropriately using the sorted array to render medications, ensuring active medications are displayed before ended ones.
526-549: Visual distinction for medication statuses needs i18n considerationThe Badge component implementation effectively distinguishes between active and ended medications. However, there's a potential i18n issue.
#!/bin/bash # Description: Verify translation keys exist for medication statuses in i18n files # Check for the "active" and "ended" keys in all translation files echo "Looking for 'active' translation key:" rg -l '"active":\s*"' --type json public/locales/ echo -e "\nLooking for 'ended' translation key:" rg -l '"ended":\s*"' --type json public/locales/Also, for consistent display in desktop layout, consider making the medication name always visible regardless of status:
- <span - className={`break-words line-clamp-2 hidden lg:block ${ - medication.status === "ended" ? "text-gray-500" : "" - }`} - > + <span + className={`break-words line-clamp-2 hidden lg:block ${ + medication.status === "ended" ? "text-gray-500" : "" + }`} + > {medication.medication?.display} </span>
…estion.tsx Co-authored-by: Rithvik Nishad <[email protected]>
| @@ -182,6 +183,12 @@ export function MedicationRequestQuestion({ | |||
| questionnaireResponse.question_id, | |||
| ); | |||
| }; | |||
| const sortedMedications = [...medications].sort((a, b) => { | |||
There was a problem hiding this comment.
Current logic would result in "active", "ended" and then everything else.
| {medication.medication?.display} | ||
| </span> | ||
| <span> | ||
| {(medication.status === "active" || |
There was a problem hiding this comment.
Same here, only showing badge for active or ended.
There was a problem hiding this comment.
Complete the merge checklist as well.
|
@Sulochan-khadka Any updates? |
ETA : EOD |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit