Updated the User Management Design #9954
Conversation
WalkthroughThis pull request introduces updates to the user management interface across multiple files. The changes enhance localization by adding new keys, refactor the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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. |
|
User search is already assigned to a different contributor, kindly refrain from taking up other issues without getting them assigned first 😅 |
@Jacobjeevan Actually the User Search issue needs to be fix to test the design |
|
@rithviknishad @Jacobjeevan Ready for Review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/components/Facility/FacilityUsers.tsx (2)
Line range hint
47-76: Consider extracting the skeleton loader component.The skeleton loader implementation is well-structured but could be moved to a separate component for reusability across the application.
+// src/components/Common/UserSkeleton.tsx +export const UserSkeleton = () => ( + <Card> + <CardContent className="p-6"> + <div className="flex items-start"> + <Skeleton className="h-16 w-16 rounded-lg mr-4" /> + <div className="flex-1"> + <div className="flex justify-between items-start"> + <div> + <Skeleton className="h-6 w-24 mb-1" /> + <Skeleton className="h-4 w-12" /> + </div> + <Skeleton className="h-6 w-16" /> + </div> + <div className="mt-2"> + <Skeleton className="h-4 w-20 mb-1" /> + <Skeleton className="h-4 w-12" /> + </div> + </div> + </div> + </CardContent> + </Card> +); // In FacilityUsers.tsx -// Current skeleton implementation +import { UserSkeleton } from "@/components/Common/UserSkeleton"; +usersList = ( + <div> + <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-2 gap-4"> + {Array.from({ length: 6 }).map((_, i) => ( + <UserSkeleton key={i} /> + ))} + </div> + </div> +);
89-126: Enhance accessibility for search and navigation.While the UI components are well-structured, consider these accessibility improvements:
<Input id="search-by-username" name="username" + aria-label={t("search_by_username")} onChange={(e) => updateQuery({ username: e.target.value })} value={qParams.username} placeholder={t("search_by_username")} className="w-full max-w-sm" /> <Tabs value={activeTab} onValueChange={(value) => setActiveTab(value as "card" | "list")} + aria-label={t("view_options")} className="ml-auto" > - <TabsList className="flex"> + <TabsList className="flex" aria-label={t("select_view_type")}>src/components/Users/UserListAndCard.tsx (3)
135-143: Improve type safety for user properties.While the null checks are good, consider using TypeScript's non-null assertion operator or optional chaining for better type safety:
-{user.user_type ? user.user_type : "-"} +{user.user_type ?? "-"} -{user.phone_number - ? formatPhoneNumber(user.phone_number) - : "-"} +{user.phone_number ? formatPhoneNumber(user.phone_number) : "-"}
246-256: Use literal types for better type safety.Consider using a literal type for the activeTab prop to prevent potential runtime errors:
interface UserListViewProps { users: UserBase[]; - activeTab: string; + activeTab: "card" | "list"; }
Line range hint
249-267: Consider memoizing components for better performance.To prevent unnecessary re-renders, consider memoizing the UserGrid and UserList components:
+import { memo } from "react"; -export const UserGrid = ({ users }: { users?: UserBase[] }) => ( +export const UserGrid = memo(({ users }: { users?: UserBase[] }) => ( <div className="grid grid-cols-1 gap-4 @xl:grid-cols-3 @4xl:grid-cols-4 @6xl:grid-cols-5 lg:grid-cols-2"> {users?.map((user) => <UserCard key={user.id} user={user} />)} </div> -); +)); -export const UserList = ({ users }: { users?: UserBase[] }) => { +export const UserList = memo(({ users }: { users?: UserBase[] }) => { // ... existing code -}; +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Facility/FacilityUsers.tsx(2 hunks)src/components/Users/UserListAndCard.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- public/locale/en.json
🔇 Additional comments (2)
src/components/Facility/FacilityUsers.tsx (2)
5-11: LGTM! Good use of standardized UI components.The changes show improved organization by:
- Using standardized UI components from a shared library
- Moving away from hardcoded values to constants
Also applies to: 18-18
29-42: Great improvements to query handling and type safety!Notable improvements:
- Added debouncing for search queries
- Explicit typing for tab state
- Well-structured pagination parameters
…05/care_fe into userList_design_update
…05/care_fe into userList_design_update
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/components/Facility/FacilityUsers.tsx (2)
Line range hint
47-85: Consider extracting the loading skeleton to a separate component.The loading state implementation is good, but extracting the skeleton to a separate component would improve maintainability and reusability.
+const UserCardSkeleton = () => ( + <Card> + <CardContent className="p-6"> + <div className="flex items-start"> + <Skeleton className="h-16 w-16 rounded-lg mr-4" /> + <div className="flex-1"> + <div className="flex justify-between items-start"> + <div> + <Skeleton className="h-6 w-24 mb-1" /> + <Skeleton className="h-4 w-12" /> + </div> + <Skeleton className="h-6 w-16" /> + </div> + <div className="mt-2"> + <Skeleton className="h-4 w-20 mb-1" /> + <Skeleton className="h-4 w-12" /> + </div> + </div> + </div> + </CardContent> + </Card> +) if (userListLoading || !userListData) { usersList = ( <div> <div className="grid grid-cols-1 md:grid-cols-2 lg:grid-cols-2 gap-4"> - {Array.from({ length: 6 }).map((_, i) => ( - <Card key={i}> - <CardContent className="p-6"> - <div className="flex items-start"> - <Skeleton className="h-16 w-16 rounded-lg mr-4" /> - <div className="flex-1"> - <div className="flex justify-between items-start"> - <div> - <Skeleton className="h-6 w-24 mb-1" /> - <Skeleton className="h-4 w-12" /> - </div> - <Skeleton className="h-6 w-16" /> - </div> - <div className="mt-2"> - <Skeleton className="h-4 w-20 mb-1" /> - <Skeleton className="h-4 w-12" /> - </div> - </div> - </div> - </CardContent> - </Card> - ))} + {Array.from({ length: 6 }).map((_, i) => ( + <UserCardSkeleton key={i} /> + ))} </div> </div> );
98-105: Consider debouncing the search input to optimize performance.While the query is debounced, debouncing the search input itself would prevent unnecessary state updates and re-renders.
+import { useDebouncedCallback } from 'use-debounce'; +const debouncedUpdateQuery = useDebouncedCallback( + (value: string) => updateQuery({ username: value }), + 300 +); <Input id="search-by-username" name="username" - onChange={(e) => updateQuery({ username: e.target.value })} + onChange={(e) => debouncedUpdateQuery(e.target.value)} value={qParams.username} placeholder={t("search_by_username")} className="w-full max-w-sm" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Facility/FacilityUsers.tsx(2 hunks)src/components/Users/UserListAndCard.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🔇 Additional comments (5)
src/components/Facility/FacilityUsers.tsx (2)
5-11: LGTM! Good improvements to the imports organization.The switch to shadcn/ui components and the use of the RESULTS_PER_PAGE_LIMIT constant improves maintainability and consistency.
Also applies to: 18-18
26-26: Well-implemented query optimization and type safety!Good improvements:
- Using debounced query to prevent excessive API calls
- Adding type safety for activeTab
- Proper pagination implementation with offset calculation
Also applies to: 29-29, 35-42
src/components/Users/UserListAndCard.tsx (3)
135-143: Good improvement in handling missing user data!The addition of fallback values for missing user_type and phone_number improves the robustness of the component.
246-246: Good improvement in type safety!The change from numeric to string literal union type for
activeTabmakes the code more type-safe and self-documenting.
249-256: Well-structured component with clear conditional rendering!The simplified logic with string literal comparison makes the code more readable and maintainable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/components/Users/UserListAndCard.tsx (1)
115-116: Use formatPhoneNumber for consistent phone number displayThe phone number should be formatted using the existing
formatPhoneNumberutility (which is already imported and used in UserListRow) for consistency.- <div className="font-medium truncate">{user.phone_number}</div> + <div className="font-medium truncate">{formatPhoneNumber(user.phone_number)}</div>src/components/Facility/FacilityUsers.tsx (3)
58-58: Consider using a constant for skeleton item countsThe magic numbers
7and6for skeleton items should be replaced with a named constant for better maintainability.+const SKELETON_ITEMS = { + LIST: 7, + CARD: 6, +}; - {Array.from({ length: 7 }).map((_, i) => ( + {Array.from({ length: SKELETON_ITEMS.LIST }).map((_, i) => ( - {Array.from({ length: 6 }).map((_, i) => ( + {Array.from({ length: SKELETON_ITEMS.CARD }).map((_, i) => (Also applies to: 91-91
167-174: Enhance search input accessibilityAdd aria-label and role attributes to improve accessibility for screen readers.
<Input id="search-by-username" name="username" + role="searchbox" + aria-label={t("search_by_username")} onChange={(e) => updateQuery({ username: e.target.value })} value={qParams.username} placeholder={t("search_by_username")} className="w-full max-w-sm" />
159-164: Improve user count loading stateThe current implementation shows an empty string while loading, which could cause layout shift. Consider showing a skeleton or placeholder.
<Badge className="bg-purple-50 text-purple-700 ml-2 text-sm font-medium rounded-xl px-3 m-3" variant="outline" > - {`${userListData ? userListData.count : ""} ${t("users")}`} + {userListData ? `${userListData.count} ${t("users")}` : <Skeleton className="h-4 w-16 bg-purple-100" />} </Badge>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/FacilityUsers.tsx(1 hunks)src/components/Users/UserListAndCard.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Users/UserListAndCard.tsx (1)
213-214: LGTM! Good architectural improvementThe simplified interface and strongly typed activeTab improve the component's API design and type safety.
Also applies to: 217-217
src/components/Facility/FacilityUsers.tsx (1)
133-137: Add validation for pagination parametersThe offset calculation assumes qParams.page is positive. Consider adding validation to ensure page numbers are positive to prevent potential issues with negative offsets.
|
LGTM |
…05/care_fe into userList_design_update
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/components/Facility/FacilityUsers.tsx (1)
165-165: 🛠️ Refactor suggestionUse correct pluralization key format.
The translation key should follow i18next's pluralization convention.
Update the translation files to use the correct key format:
{ "user_count_one": "{{count}} user", "user_count_other": "{{count}} users" }
🧹 Nitpick comments (1)
src/components/Facility/FacilityUsers.tsx (1)
21-116: Extract skeleton components into separate files.While the skeleton components are properly defined outside the main component (good!), they should be moved to separate files under a common directory for better maintainability and reusability.
Consider creating a new directory structure:
src/ components/ Skeletons/ UserCardSkeleton.tsx UserListSkeleton.tsx
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(2 hunks)src/components/Facility/FacilityUsers.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/components/Facility/FacilityUsers.tsx (2)
120-122: 🛠️ Refactor suggestionAdjust limit to match grid layout.
The limit of 15 doesn't align with the 3-column grid layout in card view. For even rows, the limit should be a multiple of 3.
const { qParams, updateQuery, Pagination } = useFilters({ - limit: 15, + limit: 18, cacheBlacklist: ["username"], });Likely invalid or redundant comment.
129-141:⚠️ Potential issueAdd error handling for the query.
The query implementation should handle error states to provide feedback to users when API calls fail.
- const { data: userListData, isLoading: userListLoading } = useQuery({ + const { data: userListData, isLoading: userListLoading, error } = useQuery({ queryKey: ["facilityUsers", facilityId, qParams], queryFn: query.debounced(routes.facility.getUsers, { pathParams: { facility_id: facilityId }, queryParams: { username: qParams.username, limit: qParams.limit, offset: (qParams.page - 1) * qParams.limit, }, }), enabled: !!facilityId, });And update the rendering logic:
- if (userListLoading || !userListData) { + if (error) { + usersList = ( + <div className="text-red-600 p-4 text-center"> + {t("error_loading_users")} + </div> + ); + } else if (userListLoading || !userListData) {Likely invalid or redundant comment.
|
@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
New Features
Bug Fixes
Refactor