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 |
rithviknishad
added
changes required
and removed
Deploy-Failed
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
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 (
|
![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
🧹 Outside diff range and nitpick comments (5)
src/components/ui/card.tsx (3)
5-18: Add JSDoc documentation for better developer experience.While the component implementation is solid, adding JSDoc documentation would improve its usability.
Add documentation like this:
+/** + * A card component that provides a consistent container with styling. + * @param className - Additional classes to be applied to the card + * @param props - HTML div element props + */ const Card = React.forwardRef< HTMLDivElement, React.HTMLAttributes<HTMLDivElement> >(({ className, ...props }, ref) => (
20-74: Consider centralizing padding management.The current implementation duplicates padding logic across CardHeader, CardContent, and CardFooter components. Consider using CSS composition or CSS custom properties to manage these spacings centrally.
Example approach:
- Define CSS custom properties for card spacings
- Use these in your components for consistent spacing management
- This would make it easier to maintain and modify spacings globally
/* In your CSS */ :root { --card-padding: 1.5rem; --card-spacing: 1.5rem; }Then use these variables in your className definitions.
76-83: Reorder exports to match declaration order.For better maintainability, consider matching the export order with the declaration order of components.
export { Card, CardHeader, CardTitle, CardDescription, CardContent, CardFooter, - CardTitle, - CardDescription, - CardContent, };src/components/Shifting/ShiftDetails.tsx (2)
432-433: Improve responsiveness of inline styles for print mode close button.The inline styles using fixed pixel values (
top-[190px],mr-[100px]) may not scale well on different screen sizes. Consider using responsive design practices with Tailwind CSS classes to ensure consistent positioning across devices.Refactor the styles using responsive utility classes:
-<div className="relative top-[190px] my-5 mr-[100px] flex justify-end gap-3 xl:absolute xl:right-6 xl:top-7 xl:justify-end"> +<div className="relative my-5 flex justify-end gap-3 xl:absolute xl:right-6 xl:top-7 xl:justify-end">Adjust margins and positioning with responsive classes like
mt-5,mr-5,top-5, etc., to maintain layout integrity.
449-449: Enhance accessibility by adding ARIA labels to buttons.To improve accessibility, add
aria-labelattributes to buttons in theoptionsprop so screen readers can convey the purpose of the buttons to users.Update the ButtonV2 components:
<ButtonV2 + aria-label={t("update_status_details")} ... <ButtonV2 + aria-label={t("referral_letter")}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Shifting/ShiftDetails.tsx(4 hunks)src/components/ui/card.tsx(1 hunks)
🔇 Additional comments (4)
src/components/ui/card.tsx (2)
1-4: LGTM! Clean and minimal imports.
The imports are well-organized and include only the necessary dependencies.
1-83: Verify integration with referral letter preview.
The card components look well-designed for improving the referral letter layout. Let's verify the integration:
✅ Verification successful
Card components are properly integrated in ShiftDetails and across the application
The verification confirms that:
- Card components are correctly imported in ShiftDetails.tsx with the necessary subcomponents (Card, CardContent, CardHeader, CardTitle)
- The components are properly used in ShiftDetails.tsx with appropriate layout classes and hierarchy
- The card components have been widely adopted across the application, indicating consistent UI patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of card components in ShiftDetails.tsx
# Expected: Find card components being used in the referral letter preview
# Search for card component imports and usage
rg -A 5 "import.*from.*card"
rg "<Card"
Length of output: 2571
src/components/Shifting/ShiftDetails.tsx (2)
305-421: Ensure dynamic data is correctly rendered in the new print layout.
The refactored printData function now uses PrintPreview and Card components for a cleaner layout. Verify that all dynamic fields, especially patient and consultation details, are accurately displayed in the printed referral letter.
Run the following script to check for placeholders or missing data bindings:
#!/bin/bash
# Description: Search for any placeholders or missing dynamic data in the printData function.
rg '\{.*\}' src/components/Shifting/ShiftDetails.tsx -A 5 -B 58-9: Verify import paths for new components.
You have added imports for PrintPreview and card components. Ensure that the paths @/CAREUI/misc/PrintPreview and @/components/ui/card are correct and that these components exist at these locations to prevent import errors.
Run the following script to confirm the existence of the imported files:
✅ Verification successful
Import paths are correct and components exist
Both imported components exist at the specified locations:
PrintPreviewcomponent exists atsrc/CAREUI/misc/PrintPreview.tsx- Card components exist at
src/components/ui/card.tsx
The imports are correctly referenced using the @/ alias which is a common pattern for absolute imports from the source directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the imported components exist.
# Verify PrintPreview component
if [ ! -f "./src/CAREUI/misc/PrintPreview.tsx" ]; then
echo "PrintPreview component not found at '@/CAREUI/misc/PrintPreview'."
else
echo "PrintPreview component found."
fi
# Verify Card components
if [ ! -f "./src/components/ui/card.tsx" ]; then
echo "Card components not found at '@/components/ui/card'."
else
echo "Card components found."
fi
Length of output: 27006
Script:
#!/bin/bash
# Let's verify the existence of the imported components using fd and rg
# Check for PrintPreview component
fd "PrintPreview.tsx" src/CAREUI/misc/
# Check for card components
fd "card.tsx" src/components/ui/
# Additionally, let's verify the actual imports in these files if they exist
rg "export.*PrintPreview" -A 2
rg "export.*Card" -A 2
Length of output: 5905
|
👋 Hi, @modamaan, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@nihal467 now only focus on desktop view, phone size i have facing issues |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
src/components/ui/card.tsx (5)
5-19: Consider enhancing accessibility attributes.While the implementation is solid, consider adding ARIA attributes to improve accessibility:
<div ref={ref} className={cn( "rounded-xl border border-gray-200 bg-white text-gray-950 shadow dark:border-gray-800 dark:bg-gray-950 dark:text-gray-50", className, )} + role="article" + aria-label="Card container" {...props} />
32-43: Consider adding heading level flexibility.The component currently uses a fixed
h3element. Consider adding a prop to allow different heading levels for better semantic structure:const CardTitle = React.forwardRef< HTMLHeadingElement, - React.HTMLAttributes<HTMLHeadingElement> + React.HTMLAttributes<HTMLHeadingElement> & { + as?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' + } ->(({ className, ...props }, ref) => ( +>(({ className, as: Comp = 'h3', ...props }, ref) => ( - <h3 + <Comp ref={ref} className={cn("font-semibold leading-none tracking-tight", className)} {...props} /> ));
56-63: Add documentation for padding behavior.Consider adding a comment to explain the
pt-0class, as it's not immediately obvious why top padding is removed:const CardContent = React.forwardRef< HTMLDivElement, React.HTMLAttributes<HTMLDivElement> >(({ className, ...props }, ref) => ( + // pt-0 is used to remove top padding when CardContent follows CardHeader <div ref={ref} className={cn("p-6 pt-0", className)} {...props} /> ));
64-74: Consider adding alignment flexibility.The footer currently uses
items-centerbut might benefit from configurable alignment:const CardFooter = React.forwardRef< HTMLDivElement, - React.HTMLAttributes<HTMLDivElement> + React.HTMLAttributes<HTMLDivElement> & { + justify?: 'start' | 'center' | 'end' | 'between' + } ->(({ className, ...props }, ref) => ( +>(({ className, justify = 'start', ...props }, ref) => ( <div ref={ref} - className={cn("flex items-center p-6 pt-0", className)} + className={cn( + "flex items-center p-6 pt-0", + { + 'justify-start': justify === 'start', + 'justify-center': justify === 'center', + 'justify-end': justify === 'end', + 'justify-between': justify === 'between', + }, + className + )} {...props} /> ));
76-83: Consider maintaining consistent export order.The export order differs slightly from the declaration order. Consider maintaining consistency:
export { Card, CardHeader, - CardFooter, CardTitle, CardDescription, CardContent, + CardFooter, };src/components/Shifting/ShiftDetails.tsx (1)
305-421: Well-structured print preview layout with room for accessibility improvementThe print preview layout is well-organized with proper responsive design. Consider adding ARIA labels to improve accessibility for screen readers.
Add aria-label to the card for better accessibility:
<Card id="section-to-print" + aria-label="Referral Letter Preview" className="print mx-auto w-full bg-white sm:mx-2 sm:my-2" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/components/Shifting/ShiftDetails.tsx(4 hunks)src/components/ui/card.tsx(1 hunks)
🔇 Additional comments (5)
src/components/ui/card.tsx (3)
1-4: LGTM! Clean and minimal imports.
The imports are well-organized and include only the necessary dependencies.
20-31: LGTM! Well-structured header component.
The component follows consistent patterns and implements appropriate spacing using flex layout.
44-55: LGTM! Semantic and accessible description component.
The component uses appropriate semantic HTML and maintains consistent styling patterns.
src/components/Shifting/ShiftDetails.tsx (2)
8-9: LGTM: New component imports
The imports for PrintPreview and Card components are correctly structured and follow the project's import conventions.
449-449: LGTM: Clean and responsive button layout
The flex container implementation provides a clean and responsive layout for the action buttons, properly handling both mobile and desktop views.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/components/Shifting/ShiftDetails.tsx (2)
321-324: Consider increasing QR code size for better scannability.The current QR code size of 120 pixels might be too small for reliable scanning, especially when printed.
<QRCodeSVG value={`${window.location.origin}/shifting/${data.id}`} - size={120} + size={160} />
338-408: Consolidate text size classes for better maintainability.The repeated text size classes (
text-xs sm:text-sm) across multiple elements could be consolidated using a parent class or a utility class.+<div className="text-xs sm:text-sm"> <p> - className="text-xs sm:text-sm" {t("name")}: {patientData?.name} </p> <p> - className="text-xs sm:text-sm" {t("age")}: {formatPatientAge(patientData, true)} </p> // ... other paragraphs +</div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Shifting/ShiftDetails.tsx(4 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-06T18:12:29.794Z
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-10: LGTM! Import changes align with the new implementation.
The new imports support the PrintPreview functionality and card-based layout structure.
Also applies to: 18-18
432-433: LGTM! Button positioning accommodates component structure.
The button layout implementation considers the component structure where the print button is inside PrintPreview and the close button is outside.
449-449: LGTM! Improved button layout and user feedback.
The flex layout provides better button alignment, and the tooltip implementation enhances user experience by explaining disabled states.
Proposed Changes
Referral letter preview desktop and responsive
Before
After
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes