-
Notifications
You must be signed in to change notification settings - Fork 533
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
Added Skeleton Loader for Care public page | Adjusted the roundness of Avatar #10124
Conversation
WalkthroughThis pull request focuses on enhancing the visual styling and loading experience across multiple components. The changes primarily involve adding rounded corners to Avatar components, updating responsive design classes, and replacing generic loading indicators with more detailed skeleton loaders. These modifications improve the user interface's consistency and provide more informative loading states in various pages like Facilities, Appointments, and Patient sections. Changes
Assessment against linked issues
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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. |
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 comments (1)
src/pages/Appointments/AppointmentsPage.tsx (1)
Line range hint
356-673
: Enhance accessibility for interactive elementsAdd appropriate ARIA labels to improve accessibility:
- Date range picker button
- Status dropdown
- Search input
Example improvements:
<Input className="w-[300px]" placeholder={t("search")} + aria-label={t("search_appointments")} value={qParams.search ?? ""} onChange={(e) => setQParams({ ...qParams, search: e.target.value })} />
🧹 Nitpick comments (6)
src/components/Common/Avatar.tsx (2)
69-69
: Review className prop handling.The use of
cn
utility for className merging is good, but the same className prop is being applied twice - once to the root element and once to the image. This could lead to unexpected styling if the parent passes classes that should only apply to one of these elements.Consider using separate className props or applying specific classes to each element:
-className={cn("aspect-square h-full w-full object-cover", className)} +className="aspect-square h-full w-full object-cover"
Line range hint
34-39
: Document className prop usage.Since the component now supports className customization, it would be helpful to document its usage in the interface.
Add JSDoc comment to describe className behavior:
interface AvatarProps { colors?: [string, string]; name: string; imageUrl?: string; + /** Additional classes to apply to the avatar root element */ className?: string; }
src/pages/Facility/FacilitiesPage.tsx (1)
109-111
: LGTM! Well-structured skeleton loader implementation.The skeleton loader implementation:
- Matches the grid layout of actual facility cards
- Uses appropriate gap spacing
- Provides visual feedback during loading state
Consider extracting the grid layout className to a constant since it's duplicated between the skeleton and actual content sections:
const GRID_LAYOUT_CLASS = "grid grid-cols-1 md:grid-cols-2 gap-4";src/pages/Patient/index.tsx (1)
67-78
: LGTM! Well-structured loading state implementation.The loading state implementation:
- Uses appropriate skeleton components for different UI elements
- Maintains visual hierarchy with header and content sections
- Provides meaningful loading feedback
Consider extracting the skeleton layout to a separate component for better reusability:
const AppointmentsSkeleton = () => ( <div> <div className="flex justify-between w-full mb-8"> <Skeleton className="h-8 w-48" /> <Skeleton className="h-8 w-48" /> </div> <Skeleton className="h-8 w-48" /> <div className="grid gap-4 mt-4"> <CardListSkeleton count={6} /> </div> </div> );src/pages/Appointments/AppointmentsPage.tsx (2)
Line range hint
10-10
: Replace placeholder token number implementationThe TODO comment indicates a temporary implementation for token numbers. I can help implement a proper token number system.
Would you like me to help implement a proper token number system or create an issue to track this task?
356-357
: Extract magic numbers into constantsThe layout calculations use hardcoded values that should be extracted into named constants for better maintainability:
h-[calc(100vh-18rem)]
h-[calc(100vh-22rem)]
Consider creating a constants file for these values:
// constants.ts export const LAYOUT = { BOARD_VIEW_HEIGHT: '18rem', LIST_VIEW_HEIGHT: '22rem', } as const;Also applies to: 671-673
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
src/components/Common/Avatar.tsx
(1 hunks)src/components/Patient/PatientDetailsTab/PatientUsers.tsx
(2 hunks)src/components/Patient/PatientInfoCard.tsx
(1 hunks)src/components/Patient/symptoms/SymptomTable.tsx
(1 hunks)src/components/Users/UserAvatar.tsx
(1 hunks)src/components/Users/UserBanner.tsx
(1 hunks)src/components/Users/UserListAndCard.tsx
(2 hunks)src/components/ui/sidebar/patient-switcher.tsx
(2 hunks)src/pages/Appointments/AppointmentsPage.tsx
(1 hunks)src/pages/Facility/FacilitiesPage.tsx
(2 hunks)src/pages/Facility/components/UserCard.tsx
(1 hunks)src/pages/FacilityOrganization/FacilityOrganizationUsers.tsx
(1 hunks)src/pages/Organization/OrganizationPatients.tsx
(1 hunks)src/pages/Organization/OrganizationUsers.tsx
(1 hunks)src/pages/Patient/index.tsx
(2 hunks)src/pages/UserDashboard.tsx
(3 hunks)
✅ Files skipped from review due to trivial changes (12)
- src/components/Users/UserBanner.tsx
- src/pages/Facility/components/UserCard.tsx
- src/components/Users/UserAvatar.tsx
- src/components/Patient/symptoms/SymptomTable.tsx
- src/pages/Organization/OrganizationPatients.tsx
- src/pages/FacilityOrganization/FacilityOrganizationUsers.tsx
- src/pages/Organization/OrganizationUsers.tsx
- src/components/Patient/PatientInfoCard.tsx
- src/components/Users/UserListAndCard.tsx
- src/components/ui/sidebar/patient-switcher.tsx
- src/pages/UserDashboard.tsx
- src/components/Patient/PatientDetailsTab/PatientUsers.tsx
🔇 Additional comments (3)
src/pages/Facility/FacilitiesPage.tsx (1)
11-11
: LGTM! Import of skeleton loader component.The addition of CardGridSkeleton aligns with the PR objective of implementing skeleton loaders for better loading states.
src/pages/Patient/index.tsx (1)
10-10
: LGTM! Appropriate skeleton component imports.The imports of both base Skeleton and CardListSkeleton components allow for flexible loading state representations.
Also applies to: 13-13
src/pages/Appointments/AppointmentsPage.tsx (1)
356-357
: Review responsive layout adjustmentsThe changes improve the layout on medium-sized screens by:
- Using
lg:flex-row
instead ofmd:flex-row
to maintain vertical stacking longer- Replacing
md:items-start
withxl:flex-row
to better handle the practitioner selection and date range componentsHowever, consider testing these breakpoints to ensure optimal spacing and alignment across all screen sizes:
- lg breakpoint (1024px) for the main container
- xl breakpoint (1280px) for the nested container
✅ Verification successful
Responsive breakpoint changes are consistent with codebase patterns
The changes to use
lg:flex-row
andxl:flex-row
align with the established responsive design patterns in the codebase, where:
- Major layout transitions commonly occur at the lg breakpoint (1024px)
- Complex nested containers often use xl breakpoint (1280px) for better spacing
These changes will provide a better experience on medium-sized screens by maintaining vertical stacking longer.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other responsive breakpoints in the codebase to ensure consistency rg -A 2 "(?:sm|md|lg|xl|2xl):" --type ts --type tsxLength of output: 81
Script:
#!/bin/bash # First, find all TypeScript/React files echo "=== TypeScript/React files ===" fd "\.(tsx|ts)$" echo -e "\n=== Responsive breakpoints usage ===" # Search for Tailwind responsive prefixes with proper context rg -C 2 "(sm|md|lg|xl|2xl):[a-zA-Z0-9-]+" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' --type tsx --type tsLength of output: 70342
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.
can't we apply rounded-md
inside the avatar components classNames itself since this is desired behaviour on all usages mostly?
Yes. |
@Rishith25 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! 🙌 |
@rithviknishad Can you assign story points to this |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
UI Improvements
Performance
Style Updates