Move all the Skeletons to seperate file #9982
Conversation
|
Warning Rate limit exceeded@AdityaJ2305 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 9 minutes and 53 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (10)
WalkthroughThis pull request introduces a comprehensive refactoring of skeleton loading components across multiple files. The primary focus is on centralizing loading state representations by creating new skeleton loading components in Changes
Assessment against linked issues
Possibly related PRs
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. |
|
Hey Team, could you add work-in-progess label as skeletons is pending for |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (2)
src/components/Patient/symptoms/list.tsx (1)
Line range hint
48-48: Missing translation for CardTitleThe title "Symptoms" is hardcoded while other instances use the translation key. This creates inconsistency in internationalization.
- <CardTitle>Symptoms</CardTitle> + <CardTitle>{t("symptoms")}</CardTitle>src/pages/Organization/OrganizationIndex.tsx (1)
Line range hint
41-42: Add translations for static textSeveral strings in the component are hardcoded and should be translated for internationalization support:
- No Organizations Found + {t("organizations.not_found")} - You don't have access to any organizations yet. + {t("organizations.no_access")} - Organizations help you manage facilities, users, and resources - efficiently. Contact your administrator to get access. + {t("organizations.help_text")} - View Details + {t("common.view_details")}Don't forget to add the translation hook:
+ const { t } = useTranslation();Also applies to: 44-45, 48-51, 85-85
🧹 Nitpick comments (4)
src/components/Patient/allergy/list.tsx (1)
Line range hint
56-71: Consider extracting badge style utilitiesThe
getStatusBadgeStyleandgetCategoryBadgeStylefunctions could be moved to a shared utility file since they might be reused across components.Create a new file
src/utils/badgeStyles.ts:export const getStatusBadgeStyle = (status: string | undefined) => { switch (status?.toLowerCase()) { case "active": return "bg-green-100 text-green-800 border-green-200"; case "inactive": return "bg-gray-100 text-gray-800 border-gray-200"; default: return "bg-gray-100 text-gray-800 border-gray-200"; } }; export const getCategoryBadgeStyle = (category: string) => { switch (category?.toLowerCase()) { case "food": return "bg-orange-100 text-orange-800 border-orange-200"; case "medication": return "bg-blue-100 text-blue-800 border-blue-200"; case "environment": return "bg-green-100 text-green-800 border-green-200"; default: return "bg-gray-100 text-gray-800 border-gray-200"; } };Also applies to: 73-84
src/components/Common/UseSkeletons.tsx (2)
15-47: Add props for customizing skeleton counts and dimensions.The skeleton components have hardcoded values for:
- Number of items (length: 3, length: 6)
- Dimensions (w-32, h-6, etc.)
Consider making these configurable through props for better reusability.
Example implementation:
- export const EncounterListSkeleton = () => { + interface SkeletonProps { + count?: number; + className?: string; + } + + export const EncounterListSkeleton = ({ count = 6, className }: SkeletonProps) => { return ( <> - {Array.from({ length: 6 }).map((_, i) => ( + {Array.from({ length: count }).map((_, i) => ( <Card key={i} className="hover:shadow-lg transition-shadow">Also applies to: 49-69, 71-85, 87-110, 112-146
148-159: Consider adding loading animation to PatientListSkeleton.The
PatientListSkeletonlacks the pulse animation present in other skeleton components. Consider addinganimate-pulsefor consistency.<CardContent> - <Skeleton className="h-[100px] w-full" /> + <Skeleton className="h-[100px] w-full animate-pulse" /> </CardContent>src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx (1)
Line range hint
151-165: Consider moving skeleton to UseSkeletons.tsx.This skeleton implementation could be moved to the centralized location for consistency with the PR's objective.
+ // In UseSkeletons.tsx + export const QuestionnaireResponseSkeleton = ({ count = 3 }: SkeletonProps) => ( + <div className="grid gap-4"> + {Array.from({ length: count }).map((_, i) => ( + <Card key={i} className="flex items-center justify-between p-4"> + <div className="flex items-start gap-4"> + <div className="h-5 w-5 animate-pulse rounded-full bg-muted" /> + <div className="space-y-2"> + <div className="h-4 w-48 animate-pulse rounded bg-muted" /> + <div className="h-3 w-32 animate-pulse rounded bg-muted" /> + <div className="h-3 w-40 animate-pulse rounded bg-muted" /> + </div> + </div> + </Card> + ))} + </div> + ); - // In QuestionnaireResponsesList.tsx - {Array.from({ length: 3 }).map((_, i) => ( - <Card key={i} className="flex items-center justify-between p-4"> - ... - </Card> - ))} + <QuestionnaireResponseSkeleton />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
public/locale/en.json(3 hunks)src/components/Common/UseSkeletons.tsx(1 hunks)src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx(1 hunks)src/components/Patient/PatientDetailsTab/patientUpdates.tsx(1 hunks)src/components/Patient/allergy/list.tsx(4 hunks)src/components/Patient/diagnosis/list.tsx(4 hunks)src/components/Patient/symptoms/list.tsx(3 hunks)src/pages/Encounters/EncounterList.tsx(2 hunks)src/pages/Encounters/tabs/EncounterNotesTab.tsx(1 hunks)src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx(2 hunks)src/pages/Organization/OrganizationIndex.tsx(2 hunks)src/pages/Organization/components/OrganizationLayout.tsx(2 hunks)
👮 Files not reviewed due to content moderation or server errors (4)
- src/pages/Encounters/tabs/EncounterNotesTab.tsx
- src/pages/Encounters/EncounterList.tsx
- src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx
- public/locale/en.json
🔇 Additional comments (2)
src/pages/Organization/components/OrganizationLayout.tsx (1)
18-18: LGTM! Clean replacement of inline skeleton.The change successfully moves the skeleton implementation to the centralized location.
Also applies to: 72-72
src/components/Patient/PatientDetailsTab/patientUpdates.tsx (1)
51-51: LGTM! Consistent array generation pattern.The change aligns with the pattern used in other skeleton components.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
src/pages/Encounters/EncounterList.tsx (1)
37-37: LGTM! Consider maintaining grid layout consistency during loading.The skeleton implementation is clean, but the grid layout might shift between loading and loaded states. Consider wrapping the
EncounterListSkeletonwith the same grid container div to maintain consistent layout:{isLoading ? ( - <EncounterListSkeleton /> + <div className="col-span-full"> + <EncounterListSkeleton /> + </div> ) : encounters.length === 0 ? (Also applies to: 669-669
src/components/Common/SkeletonComponents.tsx.tsx (7)
15-47: Add accessibility attributes and consider using consistent width variables.While the skeleton structure is well-organized, consider these improvements:
- Add
aria-busy="true"androle="status"to the skeleton container for better accessibility- Consider defining width constants or CSS variables for consistent skeleton widths across components
export const EncounterListSkeleton = () => { return ( - <> + <div aria-busy="true" role="status"> {Array.from({ length: 6 }).map((_, i) => ( <Card key={i} className="hover:shadow-lg transition-shadow">
49-69: Standardize spacing units and enhance accessibility.Consider these improvements:
- Use consistent spacing units (either all rem-based or all pixel-based)
- Add accessibility attributes as suggested for the previous component
export const FacilityOrganizationSkeleton = () => { return ( - <div className="px-6 py-6 space-y-6"> + <div className="p-6 space-y-6" aria-busy="true" role="status">
71-85: Add TypeScript return type for consistency.The component implementation is clean, but let's maintain type safety:
-export const MessageSkeleton = () => ( +export const MessageSkeleton = (): JSX.Element => (
87-110: Consider component composition to reduce duplication.This component shares similar card structure with
FacilityOrganizationSkeleton. Consider extracting a commonSkeletonCardcomponent to reduce code duplication and maintain consistency.Example:
const SkeletonCard = () => ( <Card className="relative"> <CardHeader> <Skeleton className="h-6 w-2/3" /> <Skeleton className="h-4 w-1/2" /> </CardHeader> <CardContent> <Skeleton className="h-4 w-full mb-2" /> <Skeleton className="h-4 w-3/4" /> </CardContent> <CardFooter> <Skeleton className="h-8 w-24" /> </CardFooter> </Card> );
112-146: Consider adding configuration props for flexibility.The component could benefit from configuration props to control:
- Number of items in each section (
itemCount)- Grid layout configuration (columns per breakpoint)
- Spacing customization
This would make the component more reusable across different contexts.
-export const OrganizationLayoutSkeleton = () => { +interface OrganizationLayoutSkeletonProps { + itemCount?: number; + gridCols?: { + sm?: number; + md?: number; + lg?: number; + }; +} + +export const OrganizationLayoutSkeleton = ({ + itemCount = 6, + gridCols = { sm: 1, md: 2, lg: 3 } +}: OrganizationLayoutSkeletonProps): JSX.Element => {
148-159: Add prop type validation and consider flexible height.The component could be improved by:
- Adding proper TypeScript interface for props
- Making the height configurable
- Adding prop validation
+interface PatientListSkeletonProps { + title: string; + height?: string; +} + -export const PatientListSkeleton = ({ title }: { title: string }) => { +export const PatientListSkeleton = ({ + title, + height = "100px" +}: PatientListSkeletonProps): JSX.Element => { return ( <Card> <CardHeader> <CardTitle>{title}</CardTitle> </CardHeader> <CardContent> - <Skeleton className="h-[100px] w-full" /> + <Skeleton className={`h-[${height}] w-full`} /> </CardContent> </Card> ); };
1-159: Well-structured implementation of skeleton components!The centralization of skeleton components is well-executed, with consistent patterns and good organization. The components provide a solid foundation for loading states across the application.
Some general suggestions for the entire file:
- Consider adding a README.md to document usage patterns and available customization options
- Add storybook stories to showcase different states and configurations
- Consider adding unit tests to verify proper rendering and prop handling
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/Common/SkeletonComponents.tsx.tsx(1 hunks)src/components/Patient/allergy/list.tsx(4 hunks)src/components/Patient/diagnosis/list.tsx(4 hunks)src/components/Patient/symptoms/list.tsx(3 hunks)src/pages/Encounters/EncounterList.tsx(2 hunks)src/pages/Encounters/tabs/EncounterNotesTab.tsx(1 hunks)src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx(2 hunks)src/pages/Organization/OrganizationIndex.tsx(2 hunks)src/pages/Organization/components/OrganizationLayout.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx
- src/pages/Encounters/tabs/EncounterNotesTab.tsx
- src/components/Patient/symptoms/list.tsx
🔇 Additional comments (5)
src/pages/Organization/components/OrganizationLayout.tsx (1)
18-18: LGTM! Clean implementation of centralized loading state.The changes successfully move the skeleton component to a separate file, improving code organization and maintainability.
Also applies to: 72-72
src/components/Patient/diagnosis/list.tsx (1)
19-19: Verify all required translation keys exist.The component uses several translation keys that need to be present in all locale files:
- "diagnosis"
- "diagnosis_empty_message"
#!/bin/bash # Check if translation keys exist in all locale files for key in "diagnosis" "diagnosis_empty_message"; do echo "=== Checking key: $key ===" rg "\"$key\":" public/locale/*.json doneAlso applies to: 30-30, 37-37, 40-42, 51-51
src/pages/Organization/OrganizationIndex.tsx (1)
17-17: LGTM! Clean implementation of loading state.The replacement of multiple Skeleton components with a single OrganizationIndexSkeleton improves code maintainability.
Also applies to: 33-33
src/components/Patient/allergy/list.tsx (1)
28-28: Verify all required translation keys exist.The component uses several translation keys that need to be present in all locale files:
- "allergies"
- "allergies_empty_message"
- "allergen"
- "category"
- "status"
- "criticality"
- "created_by"
#!/bin/bash # Check if translation keys exist in all locale files for key in "allergies" "allergies_empty_message" "allergen" "category" "status" "criticality" "created_by"; do echo "=== Checking key: $key ===" rg "\"$key\":" public/locale/*.json doneAlso applies to: 38-38, 45-45, 48-50, 89-93
src/components/Common/SkeletonComponents.tsx.tsx (1)
1-14: Well-organized imports!The imports are properly organized and grouped, with clear separation between external dependencies and internal components.
Jacobjeevan
added
waiting for related PR
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Organization/components/OrganizationLayout.tsx (1)
18-18: Remove the.tsxextension from the import path.The file extension in import statements is unnecessary and should be removed for consistency with TypeScript best practices.
-import { OrganizationIndexSkeleton } from "@/components/Common/SkeletonComponents.tsx"; +import { OrganizationIndexSkeleton } from "@/components/Common/SkeletonComponents";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(3 hunks)src/pages/Organization/components/OrganizationLayout.tsx(2 hunks)
🔇 Additional comments (2)
public/locale/en.json (1)
381-381: LGTM! The new empty state messages are well-structured.The added translations for empty states follow consistent naming conventions and message formatting patterns.
Also applies to: 761-761, 1974-1974
src/pages/Organization/components/OrganizationLayout.tsx (1)
72-72: LGTM! Good job centralizing the loading state.The change successfully moves the skeleton component to a separate file while maintaining proper loading state handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Common/SkeletonComponents.tsx (4)
15-47: Consider extracting the card skeleton to a reusable component.The
EncounterListSkeletonmaps over an array to create multiple identical card structures. This pattern could be reused across other components.Consider creating a reusable
CardSkeletoncomponent:interface CardSkeletonProps { headerItems?: number[]; // widths for header items contentItems?: number[]; // widths for content items showSeparator?: boolean; } const CardSkeleton = ({ headerItems = [32, 16], contentItems = [12, 20, 12, 24], showSeparator = true }: CardSkeletonProps) => ( <Card className="hover:shadow-lg transition-shadow"> <CardHeader className="space-y-1"> <div className="flex items-center justify-between"> {headerItems.map((width, i) => ( <Skeleton key={i} className={`h-6 w-${width}`} /> ))} </div> </CardHeader> <CardContent> <div className="flex flex-col space-y-2"> {contentItems.map((width, i) => ( <div key={i} className="flex items-center justify-between"> <Skeleton className={`h-4 w-${width}`} /> </div> ))} {showSeparator && <Separator className="my-2" />} </div> </CardContent> </Card> );
49-69: Consider making the skeleton count configurable.The component has a hardcoded length of 6 items. Making this configurable would improve reusability.
interface FacilityOrganizationSkeletonProps { count?: number; } export const FacilityOrganizationSkeleton = ({ count = 6 }: FacilityOrganizationSkeletonProps) => { return ( <div className="px-6 py-6 space-y-6"> <Skeleton className="h-8 w-48" /> <Skeleton className="h-8 w-48 self-end" /> <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-3 gap-4"> {Array.from({ length: count }).map((_, i) => ( // ... rest of the component ))} </div> </div> ); };
71-85: Add aria-label for accessibility.The message skeleton should indicate its loading state to screen readers.
-export const MessageSkeleton = () => ( +export const MessageSkeleton = () => ( - <div className="space-y-4"> + <div className="space-y-4" role="status" aria-label="Loading messages"> {Array.from({ length: 3 }).map((_, i) => (
148-159: Consider adding a loading indicator for screen readers.The
PatientListSkeletonshould communicate its loading state to screen readers.export const PatientListSkeleton = ({ title }: { title: string }) => { return ( - <Card> + <Card role="status" aria-label={`Loading ${title}`}> <CardHeader> <CardTitle>{title}</CardTitle> </CardHeader>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/components/Common/SkeletonComponents.tsx(1 hunks)src/components/Patient/allergy/list.tsx(4 hunks)src/components/Patient/diagnosis/list.tsx(4 hunks)src/components/Patient/symptoms/list.tsx(3 hunks)src/pages/Encounters/EncounterList.tsx(2 hunks)src/pages/Encounters/tabs/EncounterNotesTab.tsx(1 hunks)src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx(2 hunks)src/pages/Organization/OrganizationIndex.tsx(2 hunks)src/pages/Organization/components/OrganizationLayout.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/pages/FacilityOrganization/FacilityOrganizationIndex.tsx
- src/components/Patient/diagnosis/list.tsx
- src/pages/Encounters/tabs/EncounterNotesTab.tsx
- src/pages/Organization/OrganizationIndex.tsx
- src/components/Patient/allergy/list.tsx
- src/components/Patient/symptoms/list.tsx
- src/pages/Encounters/EncounterList.tsx
🔇 Additional comments (4)
src/components/Common/SkeletonComponents.tsx (2)
1-14: LGTM! Well-organized imports.The imports are properly structured and all are being utilized in the implementation.
87-110: LGTM! Well-implemented i18n support.The component properly uses the translation hook for the title, making it internationalization-ready.
src/pages/Organization/components/OrganizationLayout.tsx (2)
18-18: LGTM! Clean import of the skeleton component.The import follows the project's import organization pattern.
72-72: LGTM! Proper implementation of the skeleton component.The loading state is handled correctly using the imported skeleton component.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/components/Common/SkeletonLoading.tsx (1)
4-57: 🛠️ Refactor suggestionAdd ARIA attributes and consistent animations to TableSkeleton.
The component needs accessibility improvements and consistent animations.
export const TableSkeleton = ({ count }: { count: number }) => ( - <div className="overflow-x-auto rounded-lg border border-gray-200"> + <div + className="overflow-x-auto rounded-lg border border-gray-200" + role="status" + aria-label="Loading table content" + > <table className="relative min-w-full divide-y divide-gray-200"> {/* Header Skeleton */} - <thead> + <thead className="animate-pulse">Also, consider adding type constraints for the count prop:
interface TableSkeletonProps { count: number & { readonly brand: unique symbol } }
🧹 Nitpick comments (1)
src/components/Common/SkeletonLoading.tsx (1)
74-111: Improve CardGridSkeleton accessibility and maintainability.
- Add ARIA attributes for accessibility
- Consider extracting card content for better maintainability
export const CardGridSkeleton = ({ count }: { count: number }) => Array.from({ length: count }, (_, index) => ( - <div key={index} className="skeleton-item animate-pulse"> + <div + key={index} + className="skeleton-item animate-pulse" + role="status" + aria-label={`Loading card ${index + 1} of ${count}`} + >Consider extracting the card content into a separate component:
const CardContent = () => ( <div className="flex flex-col h-full gap-4"> {/* ... existing card content ... */} </div> ); export const CardGridSkeleton = ({ count }: { count: number }) => Array.from({ length: count }, (_, index) => ( <div key={index} className="skeleton-item animate-pulse" role="status" aria-label={`Loading card ${index + 1} of ${count}`} > <Card className="h-full"> <CardContent className="p-4 sm:p-6"> <CardContent /> </CardContent> </Card> </div> ));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/SkeletonLoading.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/components/Common/SkeletonLoading.tsx (2)
1-2: LGTM! Clean and necessary imports.
1-111: Consider restructuring skeleton components for better modularity.Based on the team's discussion and bodhish's feedback, consider moving skeleton components to their respective feature modules:
- Create generic base skeletons in this file
- Move feature-specific implementations to their respective directories
For example:
src/ ├── components/ │ └── Common/ │ └── SkeletonLoading.tsx (generic base components) └── pages/ ├── Users/ │ └── components/ │ └── UserTableSkeleton.tsx └── Notes/ └── components/ └── NoteListSkeleton.tsx
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/components/Common/SkeletonLoading.tsx (2)
4-57: 🛠️ Refactor suggestionAdd ARIA attributes for better accessibility.
The table skeleton should include proper ARIA attributes for screen reader support.
export const TableSkeleton = ({ count }: { count: number }) => ( - <div className="overflow-x-auto rounded-lg border border-gray-200"> + <div + className="overflow-x-auto rounded-lg border border-gray-200" + role="status" + aria-label="Loading table content" + > <table className="relative min-w-full divide-y divide-gray-200">Apply animate-pulse consistently.
The header section should also have the animate-pulse class for visual consistency.
- <thead> + <thead className="animate-pulse">Consider pagination for large tables.
For better performance with large datasets, consider implementing pagination or limiting the maximum number of skeleton rows.
export const TableSkeleton = ({ count }: { count: number }) => ( + // Limit maximum rows to prevent performance issues + const safeCount = Math.min(count, 10); <div className="overflow-x-auto rounded-lg border border-gray-200"> {/* ... */} - {Array.from({ length: count }).map((_, i) => ( + {Array.from({ length: safeCount }).map((_, i) => (
59-75: 🛠️ Refactor suggestionEnhance accessibility for note skeletons.
Add ARIA attributes to improve screen reader experience.
export const NoteSkeleton = ({ count }: { count: number }) => ( - <div className="space-y-4"> + <div + className="space-y-4" + role="status" + aria-label="Loading notes" + > {Array.from({ length: count }, (_, index) => ( - <div key={index}> + <div + key={index} + aria-label={`Loading note ${index + 1} of ${count}`} + >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/SkeletonLoading.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/components/Common/SkeletonLoading.tsx (2)
1-2: LGTM! Clean and necessary imports.
1-114: Consider restructuring skeleton components for better modularity.Based on the team's discussion and bodhish's feedback, consider moving skeleton components to their respective feature modules while keeping shared utilities in this file.
- Create a base skeleton utilities file:
// src/components/Common/SkeletonUtils.tsx export const createTableSkeleton = (columns: number, rows: number) => {...} export const createCardSkeleton = (config: CardSkeletonConfig) => {...} export const createNoteSkeleton = (config: NoteSkeletonConfig) => {...}
- Move feature-specific implementations to their respective modules:
// src/pages/Users/components/UserTableSkeleton.tsx import { createTableSkeleton } from '@/components/Common/SkeletonUtils' export const UserTableSkeleton = () => createTableSkeleton(5, 10) // src/pages/Notes/components/PatientNoteSkeleton.tsx import { createNoteSkeleton } from '@/components/Common/SkeletonUtils' export const PatientNoteSkeleton = () => createNoteSkeleton({ count: 3 })This approach:
- Improves maintainability by co-locating related code
- Reduces coupling between features
- Makes it easier to modify individual skeletons
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
@AdityaJ2305 is this PR ready for testing |
Yes |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Common/SkeletonLoading.tsx (4)
4-57: Add TypeScript interface for component props.Define a proper interface for the component props to improve type safety.
+interface TableSkeletonProps { + count: number; + /** Optional number of columns. Defaults to 5 */ + columns?: number; +} + -export const TableSkeleton = ({ count }: { count: number }) => ( +export const TableSkeleton = ({ count, columns = 5 }: TableSkeletonProps) => (
28-28: Apply animate-pulse consistently.The animation is only applied to tbody, while other skeleton components apply it to the root element.
- <tbody className="divide-y divide-gray-200 bg-white animate-pulse"> + <tbody className="divide-y divide-gray-200 bg-white">Move animate-pulse to the root div:
- <div className="overflow-x-auto rounded-lg border border-gray-200"> + <div className="overflow-x-auto rounded-lg border border-gray-200 animate-pulse">
64-64: Use Skeleton component consistently.Replace direct background color usage with the Skeleton component for consistency.
- <div className="w-8 h-8 rounded-full bg-gray-200" /> + <Skeleton className="w-8 h-8 rounded-full" />
74-111: Consider performance optimization for large counts.The component creates a complex DOM structure for each item. For large counts, this could impact performance.
Consider:
- Adding a maximum count limit
- Using CSS grid for layout instead of nested flexbox
- Implementing virtualization for large lists
+const MAX_SKELETON_COUNT = 20; + -export const CardGridSkeleton = ({ count }: { count: number }) => +export const CardGridSkeleton = ({ count }: { count: number }) => { + const safeCount = Math.min(count, MAX_SKELETON_COUNT); + return ( // ... rest of the component + ); +};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(3 hunks)src/components/Common/SkeletonLoading.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🔇 Additional comments (5)
src/components/Common/SkeletonLoading.tsx (5)
1-2: LGTM! Clean imports using proper absolute paths.
4-57: Add ARIA attributes to improve accessibility.This was previously suggested in the past review comments. The component needs proper ARIA attributes for better screen reader support.
59-72: Improve NoteSkeleton structure and accessibility.This was previously suggested in the past review comments. The component needs:
- A wrapper element for the array
- ARIA attributes for accessibility
74-111: Improve CardGridSkeleton structure and accessibility.This was previously suggested in the past review comments. The component needs:
- A wrapper element for the array
- ARIA attributes for accessibility
1-111: Consider restructuring skeleton components for better modularity.As discussed in the PR comments by bodhish, consider moving each skeleton component to its respective feature module:
TableSkeleton→ Common component (can stay here as it's generic)NoteSkeleton→src/pages/Notes/components/CardGridSkeleton→ Create separate card skeletons for each feature using this as a baseThis would improve maintainability and align with the team's suggested architecture.
src/components/Facility/ConsultationDetails/QuestionnaireResponsesList.tsx
Outdated
Show resolved
Hide resolved
|
@AdityaJ2305 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Localization
UI/UX Improvements
TableSkeleton,NoteSkeleton,CardGridSkeleton) for consistent loading states across multiple pages.Internationalization
useTranslationhook in organization-related components.