-
Notifications
You must be signed in to change notification settings - Fork 447
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
Referral letter preview desktop #8954
base: develop
Are you sure you want to change the base?
Referral letter preview desktop #8954
Conversation
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
…ui/separator.tsx. to resolve deploy issue
@bodhish should i update anything? |
@rithviknishad can you check |
|
@khavinshankar can you check |
@modamaan could you resolve conflicts |
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: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/CAREUI/misc/PrintPreview.tsx
(2 hunks)src/components/ui/separator.tsx
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/components/ui/separator.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/CAREUI/misc/PrintPreview.tsx
[error] 34-34: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/CAREUI/misc/PrintPreview.tsx (2)
34-34
: 🛠️ Refactor suggestion
Use optional chaining for better code safety
If these changes must stay (which is not recommended), use optional chaining for better code safety.
-onClick={() => props.closeFeature && props.closeFeature(false)}
+onClick={() => props.closeFeature?.(false)}
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
Line range hint 1-54
: Verify the impact on existing usages
Let's check how many other components are using PrintPreview
and could be affected by these changes.
✅ Verification successful
Changes to PrintPreview component have potential impact on multiple components
The verification shows that PrintPreview
is used in several components:
src/components/Shifting/ShiftDetails.tsx
src/components/Medicine/PrintPreview.tsx
src/components/Facility/TreatmentSummary.tsx
src/components/Facility/Investigations/InvestigationsPrintPreview.tsx
All these components use the PrintPreview
with compatible props:
title
(required): All components provide itonClose
(optional): Used by ShiftDetailscloseFeature
(optional): Used with onClose in ShiftDetailschildren
(required): All components provide contentclassName
(optional): No direct impactdisabled
(optional): No direct impact
The changes in the component appear safe as they maintain backward compatibility with existing usages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for all usages of PrintPreview component
echo "Searching for PrintPreview usages..."
rg --type tsx --type ts "PrintPreview" -B 2 -A 2
Length of output: 159
Script:
#!/bin/bash
# Search for PrintPreview usages with ripgrep, without file type filters
echo "Searching for PrintPreview usages..."
rg "PrintPreview" -B 2 -A 2
# Also search for the file name itself to understand its location
echo -e "\nSearching for PrintPreview file..."
fd "PrintPreview.tsx"
Length of output: 9382
🧰 Tools
🪛 Biome (1.9.4)
[error] 34-34: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
@rithviknishad Create a new ReferralLetterPreview component that wraps PrintPreview, shall i do it like this |
The problem is that we need both the print button and the close button in a single div to manage responsiveness. However, the print button is inside the PrintPreview component, while the close button is outside the component. |
The breadcrumbs providing back button is equivalent of having the close button as both does the same thing. So we can remove the close button since it's redundant. |
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
🧹 Outside diff range and nitpick comments (2)
src/components/Shifting/ShiftDetails.tsx (2)
280-281
: Remove development comment.The "deploytest" comment should be removed as it appears to be a development artifact.
- // deploytest
419-419
: Consider adding explicit type for printData argument.The
printData
function's argument would benefit from an explicit type definition for better type safety.- <div className="my-4">{printData(data)}</div> + <div className="my-4">{printData(data as ShiftingDetails)}</div>Consider adding a type definition:
interface ShiftingDetails { id: string; patient_object: PatientModel; is_kasp: boolean; // ... other properties }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Shifting/ShiftDetails.tsx
(3 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Shifting/ShiftDetails.tsx (1)
Learnt from: modamaan
PR: ohcnetwork/care_fe#8954
File: src/components/Shifting/ShiftDetails.tsx:432-433
Timestamp: 2024-11-12T10:23:10.322Z
Learning: In `src/components/Shifting/ShiftDetails.tsx`, the `print` button is inside the `PrintPreview` component and the `close` button is outside, so aligning them with flexbox within the same container isn't feasible.
🔇 Additional comments (3)
src/components/Shifting/ShiftDetails.tsx (3)
8-13
: LGTM! Import changes align with the UI modernization.
The switch to QRCodeSVG
and addition of new UI components improve the codebase's modularity and maintainability.
282-408
: LGTM! Well-structured print layout implementation.
The new Card-based layout provides a clean and organized structure for the referral letter. The component hierarchy and spacing are well thought out.
361-365
:
Add null check for consultation object.
The date formatting could throw an error if the consultation object is undefined.
- {formatDateTime(
- consultation.encounter_date || consultation.created_date,
- ) || "-"}
+ {consultation
+ ? formatDateTime(
+ consultation.encounter_date || consultation.created_date
+ )
+ : "-"}
Likely invalid or redundant comment.
Proposed Changes
Referral letter preview desktop and responsive
Before
After
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
User Interface