Updated Test Details page form UI #9138
Conversation
❌ Deploy Preview for care-ohc failed.
|
WalkthroughThe changes in this pull request primarily enhance the 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)
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: 3
🧹 Outside diff range and nitpick comments (6)
src/components/ui/separator.tsx (1)
6-26: Add JSDoc documentation and consider accessibility improvements.While the implementation is solid, consider these enhancements:
- Add JSDoc documentation to describe the component and its props
- Consider handling non-decorative cases with proper ARIA attributes
Add documentation and improve accessibility with this diff:
+/** + * A visual separator component that can be either horizontal or vertical. + * @param className - Additional classes to apply to the separator + * @param orientation - The orientation of the separator ('horizontal' | 'vertical') + * @param decorative - Whether the separator is purely decorative + */ const Separator = React.forwardRef< React.ElementRef<typeof SeparatorPrimitive.Root>, React.ComponentPropsWithoutRef<typeof SeparatorPrimitive.Root> >( ( { className, orientation = "horizontal", decorative = true, ...props }, ref, ) => ( <SeparatorPrimitive.Root ref={ref} decorative={decorative} + aria-orientation={orientation} + role={decorative ? "none" : "separator"} orientation={orientation} className={cn( "shrink-0 bg-gray-200 dark:bg-gray-800", orientation === "horizontal" ? "h-[1px] w-full" : "h-full w-[1px]", className, )} {...props} /> ), );src/components/ui/badge.tsx (1)
6-26: Consider adjusting warning variant hover state for better contrast.While the badge variants are well-defined with good semantic meaning and dark mode support, the warning variant's hover state (
hover:bg-yellow-500) might not provide sufficient contrast with the text color in light mode.Consider this adjustment:
warning: - "border-transparent bg-yellow-400 text-gray-900 shadow hover:bg-yellow-500 dark:bg-yellow-400 dark:text-gray-900 dark:hover:bg-yellow-500", + "border-transparent bg-yellow-400 text-gray-900 shadow hover:bg-yellow-400/80 dark:bg-yellow-400 dark:text-gray-900 dark:hover:bg-yellow-400/80",src/components/ui/card.tsx (2)
5-18: Consider enhancing accessibility with semantic HTML and ARIA attributes.While the implementation is solid, consider these improvements for better accessibility:
const Card = React.forwardRef< HTMLDivElement, React.HTMLAttributes<HTMLDivElement> >(({ className, ...props }, ref) => ( - <div + <article 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="Content card" {...props} - /> + /> ));
1-83: Well-structured compound component system.The card component system follows the compound component pattern effectively, providing a flexible and maintainable solution for the Sample Test Details page UI update. The consistent implementation of forwarded refs and className handling makes these components highly reusable.
Consider documenting usage examples in comments or creating a Storybook story to showcase different card compositions and variants.
src/components/Patient/SampleDetails.tsx (2)
292-308: Consider enhancing accessibility of status indicatorsThe new card header layout effectively organizes status and result information. However, consider adding ARIA labels for screen readers.
- <Badge variant="outline" className="font-semibold"> + <Badge variant="outline" className="font-semibold" aria-label={`Status: ${sampleDetails?.status}`}> {sampleDetails?.status} </Badge> - <Badge variant="secondary" className="font-semibold"> + <Badge variant="secondary" className="font-semibold" aria-label={`Result: ${sampleDetails?.result}`}> {sampleDetails?.result} </Badge>
450-473: Clean up redundant whitespace charactersThe layout and spacing using Separator components is good, but there are unnecessary whitespace characters in the JSX that should be removed for cleaner code.
- {" "} {sampleDetails.patient_travel_history}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/components/Patient/SampleDetails.tsx(2 hunks)src/components/ui/badge.tsx(1 hunks)src/components/ui/card.tsx(1 hunks)src/components/ui/separator.tsx(1 hunks)
🔇 Additional comments (10)
src/components/ui/separator.tsx (3)
1-4: LGTM! Well-organized imports.
The imports are properly structured with explicit imports and follow TypeScript best practices.
27-29: LGTM! Proper component naming and export.
The display name is correctly set for debugging purposes, and the export follows best practices.
1-29: Verify integration with Sample Test Details page.
Since this component is part of the UI improvements mentioned in issue #9120, let's verify its integration.
✅ Verification successful
Separator component is properly integrated and used
The verification shows that:
- The Separator component is correctly imported from the UI components
- It's being used appropriately in SampleDetails.tsx to create visual separation between different sections
- The component is used with default styling without any conflicting overrides
- The implementation aligns with the UI improvements mentioned in the PR objectives
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the Separator component usage in Sample Test Details page
# Check for Separator component usage
rg -l "Separator" "src/components/Patient/"
# Check for proper imports
rg "from.*separator" "src/components/Patient/"
Length of output: 225
Script:
#!/bin/bash
# Check the actual usage of Separator in SampleDetails.tsx
rg -A 5 "Separator" "src/components/Patient/SampleDetails.tsx"
# Check if there are any styling conflicts or overrides
rg "className.*Separator" "src/components/Patient/SampleDetails.tsx"
# Check the overall structure of SampleDetails component
ast-grep --pattern 'export function SampleDetails() {
$$$
}'
Length of output: 1070
src/components/ui/badge.tsx (4)
1-4: LGTM! Clean and well-organized imports.
The imports are properly structured, using named imports where appropriate and following React best practices.
28-30: LGTM! Well-typed interface definition.
The interface properly extends HTML div attributes and variant props, providing good type safety.
38-38: LGTM! Clean exports.
Appropriate exports of both the component and variants for maximum flexibility.
1-38: Verify Badge component integration in Sample Test Details page.
The component looks well-implemented, but let's verify its integration with the Sample Test Details page to ensure it meets the UI update requirements from issue #9120.
✅ Verification successful
Badge component is properly integrated in Sample Test Details page
The Badge component is correctly imported and extensively used throughout the Sample Test Details page with appropriate variants:
- Status display using
variant="outline" - Result display using
variant="secondary" - Multiple data fields using
variant="secondary"for consistent styling
The implementation aligns well with the UI update requirements, showing proper integration for displaying various sample test information in a visually consistent manner.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Badge component usage in Sample Test Details page
# Expected: Badge component is properly imported and used in the Sample Test Details page
# Check for Badge component imports
echo "Checking Badge component imports..."
rg -l "import.*Badge.*from.*badge" src/
# Check for Badge component usage
echo "Checking Badge component usage..."
ast-grep --pattern 'Badge $$$' src/
Length of output: 278
Script:
#!/bin/bash
# Let's examine how the Badge component is used in SampleDetails.tsx
echo "Checking Badge usage in SampleDetails.tsx..."
rg -A 5 -B 5 "Badge" src/components/Patient/SampleDetails.tsx
# Also check the specific variant being used
echo "Checking Badge variant usage..."
ast-grep --pattern '<Badge variant="$_" $$$>' src/components/Patient/SampleDetails.tsx
Length of output: 8897
src/components/ui/card.tsx (2)
1-4: LGTM! Clean and minimal imports.
The imports are well-organized and include only the necessary dependencies.
76-83: LGTM! Well-organized exports.
The named exports are cleanly organized and follow a logical order.
src/components/Patient/SampleDetails.tsx (1)
6-8: LGTM: New UI component imports align with redesign goals
The addition of Badge, Card, and Separator components provides the necessary building blocks for the improved UI layout.
| const CardTitle = React.forwardRef< | ||
| HTMLDivElement, | ||
| React.HTMLAttributes<HTMLDivElement> | ||
| >(({ className, ...props }, ref) => ( | ||
| <div | ||
| ref={ref} | ||
| className={cn("font-semibold leading-none tracking-tight", className)} | ||
| {...props} | ||
| /> | ||
| )); | ||
| CardTitle.displayName = "CardTitle"; | ||
|
|
||
| const CardDescription = React.forwardRef< | ||
| HTMLDivElement, | ||
| React.HTMLAttributes<HTMLDivElement> | ||
| >(({ className, ...props }, ref) => ( | ||
| <div | ||
| ref={ref} | ||
| className={cn("text-sm text-gray-500 dark:text-gray-400", className)} | ||
| {...props} | ||
| /> | ||
| )); | ||
| CardDescription.displayName = "CardDescription"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance semantic structure of title and description components.
Consider using semantic HTML elements for better accessibility and SEO:
const CardTitle = React.forwardRef<
- HTMLDivElement,
+ HTMLHeadingElement,
- React.HTMLAttributes<HTMLDivElement>
+ React.HTMLAttributes<HTMLHeadingElement>
>(({ className, ...props }, ref) => (
- <div
+ <h2
ref={ref}
className={cn("font-semibold leading-none tracking-tight", className)}
{...props}
- />
+ />
));
const CardDescription = React.forwardRef<
HTMLDivElement,
React.HTMLAttributes<HTMLDivElement>
>(({ className, ...props }, ref) => (
<div
ref={ref}
className={cn("text-sm text-gray-500 dark:text-gray-400", className)}
+ aria-description="true"
{...props}
/>
));Committable suggestion skipped: line range outside the PR's diff.
| {yesOrNoBadge(sampleDetails?.is_atypical_presentation)} | ||
| </div> | ||
| <div> | ||
| <span className="font-semibold leading-relaxed"> | ||
| Is unusual course{" "} | ||
| </span> | ||
| {yesOrNoBadge(sampleDetails?.is_unusual_course)} | ||
| </div> | ||
| {sampleDetails?.atypical_presentation && ( | ||
| <div className="md:col-span-2"> | ||
| <span className="font-semibold leading-relaxed"> | ||
| Atypical presentation details:{" "} | ||
| </span> | ||
| {sampleDetails.atypical_presentation} | ||
| <div className="flex justify-between items-center gap-2 p-3 bg-gray-100 rounded-lg"> | ||
| <div className="font-medium text-sm">Is unusual course </div> | ||
| <Badge variant={"secondary"}> | ||
| {" "} | ||
| {yesOrNoBadge(sampleDetails?.is_unusual_course)} | ||
| </Badge> | ||
| </div> | ||
| )} | ||
| <div> | ||
| <span className="font-semibold leading-relaxed"> | ||
| SARI - Severe Acute Respiratory illness{" "} | ||
| </span> | ||
| {yesOrNoBadge(sampleDetails?.has_sari)} | ||
| </div> | ||
| <div> | ||
| <span className="font-semibold leading-relaxed"> | ||
| ARI - Acute Respiratory illness{" "} | ||
| </span> | ||
| {yesOrNoBadge(sampleDetails?.has_ari)} | ||
| </div> | ||
| <div> | ||
| <span className="font-semibold leading-relaxed"> | ||
| Contact with confirmed carrier{" "} | ||
| </span> | ||
| {yesOrNoBadge(sampleDetails?.patient_has_confirmed_contact)} | ||
| </div> | ||
| <div> | ||
| <span className="font-semibold leading-relaxed"> | ||
| Contact with suspected carrier{" "} | ||
| </span> | ||
| {yesOrNoBadge(sampleDetails?.patient_has_suspected_contact)} | ||
| </div> | ||
| {sampleDetails?.patient_travel_history && | ||
| sampleDetails.patient_travel_history.length !== 0 && ( | ||
| <div className="md:col-span-2"> | ||
| <span className="font-semibold leading-relaxed"> | ||
| Countries travelled:{" "} | ||
| </span> | ||
| {sampleDetails.patient_travel_history} | ||
| {sampleDetails?.atypical_presentation && ( | ||
| <div className="flex justify-between items-center gap-2 p-3 bg-gray-100 rounded-lg"> | ||
| <div className="font-medium text-sm"> | ||
| Atypical presentation details:{" "} | ||
| </div> | ||
| <Badge variant={"secondary"}> | ||
| {" "} | ||
| {sampleDetails.atypical_presentation} | ||
| </Badge> | ||
| </div> | ||
| )} | ||
| <div className="flex justify-between items-center gap-2 p-3 bg-gray-100 rounded-lg"> | ||
| <div className="font-medium text-sm"> | ||
| SARI - Severe Acute Respiratory illness{" "} | ||
| </div> | ||
| <Badge variant={"secondary"}> | ||
| {" "} | ||
| {yesOrNoBadge(sampleDetails?.has_sari)} | ||
| </Badge> | ||
| </div> | ||
| <div className="flex justify-between items-center gap-2 p-3 bg-gray-100 rounded-lg"> | ||
| <div className="font-medium text-sm"> | ||
| ARI - Acute Respiratory illness{" "} | ||
| </div> | ||
| <Badge variant={"secondary"}> | ||
| {" "} | ||
| {yesOrNoBadge(sampleDetails?.has_ari)} | ||
| </Badge> | ||
| </div> | ||
| <div className="flex justify-between items-center gap-2 p-3 bg-gray-100 rounded-lg"> | ||
| <div className="font-medium text-sm"> | ||
| Contact with confirmed carrier{" "} | ||
| </div> | ||
| <Badge variant={"secondary"}> | ||
| {" "} | ||
| {yesOrNoBadge(sampleDetails?.patient_has_confirmed_contact)} | ||
| </Badge> | ||
| </div> | ||
| <div className="flex justify-between items-center gap-2 p-3 bg-gray-100 rounded-lg"> | ||
| <div className="font-medium text-sm"> | ||
| Contact with suspected carrier{" "} | ||
| </div> | ||
| <Badge variant={"secondary"}> | ||
| {" "} | ||
| {yesOrNoBadge(sampleDetails?.patient_has_suspected_contact)} | ||
| </Badge> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract repeated badge patterns into a reusable component
The grid layout effectively organizes information, but there's significant repetition in the badge patterns. Consider extracting these into a reusable component for better maintainability.
+ const InfoBadge = ({ label, value, variant = "secondary" }) => (
+ <div className="flex justify-between items-center gap-2 p-3 bg-gray-100 rounded-lg">
+ <div className="font-medium text-sm">{label}</div>
+ <Badge variant={variant}>{value}</Badge>
+ </div>
+ );
// Usage example:
- <div className="flex justify-between items-center gap-2 p-3 bg-gray-100 rounded-lg">
- <div className="font-medium text-sm">Fast track testing reason: </div>
- <Badge variant={"secondary"}>{sampleDetails.fast_track}</Badge>
- </div>
+ <InfoBadge
+ label="Fast track testing reason"
+ value={sampleDetails.fast_track}
+ />Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
vite.config.mts(1 hunks)
🔇 Additional comments (1)
vite.config.mts (1)
170-173: Verify potential duplicate dependencies
The addition of @radix-ui/react-separator to optimizeDeps.include is correct. However, let's verify it's not already included via getPluginDependencies() to avoid duplication.
✅ Verification successful
No duplicate dependency found - configuration is correct
Based on the verification:
@radix-ui/react-separatoris not present in the main package.json dependencies- The
getPluginDependencies()function scans theappsdirectory for plugin dependencies, but the directory doesn't exist in this repository - The dependency is correctly added to both
optimizeDeps.includeandexternalconfigurations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if @radix-ui/react-separator is already included in plugin dependencies
# Search for @radix-ui/react-separator in package.json files under apps directory
fd package.json apps -x jq -r '.dependencies | select(."@radix-ui/react-separator" != null) | "@radix-ui/react-separator"'
Length of output: 216
Script:
#!/bin/bash
# Let's first check the repository structure to locate package.json files
fd package.json
# Then examine the getPluginDependencies function implementation
ast-grep --pattern 'function getPluginDependencies() {
$$$
}'
# Also search for any direct references to this dependency
rg "@radix-ui/react-separator" -A 2 -B 2
Length of output: 764
Script:
#!/bin/bash
# Let's check the content of package.json for dependencies
cat package.json | jq '.dependencies'
# And examine the getPluginDependencies function content
rg -A 10 "function getPluginDependencies"
Length of output: 2078
| }, | ||
| build: { | ||
| outDir: "build", | ||
| assetsDir: "bundle", | ||
| sourcemap: true, | ||
| rollupOptions: { | ||
| external: ["@radix-ui/react-separator"], |
There was a problem hiding this comment.
Remove external configuration for Radix UI component
The configuration appears inconsistent. Marking @radix-ui/react-separator as external while including it in optimizeDeps could lead to issues:
- It prevents bundling of the dependency, which is typically not desired for UI components
- It may cause unnecessary external requests at runtime
- It contradicts the pre-bundling setup in
optimizeDeps
Remove the external configuration:
- external: ["@radix-ui/react-separator"],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| external: ["@radix-ui/react-separator"], |
|
👋 Hi, @modamaan, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
|
@modamaan check through the coderabbit suggestion , fix the issues with the PR, and once the PR is ready for review Tag me |
Proposed Changes
older
updated
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Badgecomponent for improved status and result display.Cardsystem with structured layouts for displaying sample details.Separatorcomponent for better visual distinction between sections.Improvements
SampleDetailscomponent.