Fix: Redesign the User view of Department/Teams #10568
Conversation
WalkthroughThis pull request adds several localization keys in the English locale JSON file to support department/team and organization functionalities. It also introduces a search feature within the FacilityOrganizationUsers component to filter user listings and updates the UI with a skeleton loader during data fetching. Additionally, the CreateFacilityOrganizationSheet component now leverages i18next for internationalizing user interface strings, updating messages, button texts, and placeholders accordingly. Changes
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. |
![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
🧹 Nitpick comments (4)
src/pages/Facility/settings/organizations/components/CreateFacilityOrganizationSheet.tsx (1)
66-66: Verify error message consistency.The success and error messages use different translation key patterns:
- Success:
organization_created_successfully- Error:
please_enter_organization_nameConsider standardizing the naming pattern for translation keys to improve maintainability.
Also applies to: 82-82
src/pages/Facility/settings/organizations/components/FacilityOrganizationLayout.tsx (1)
159-170: Consider accessibility improvements for tab navigation.The tab navigation could benefit from ARIA attributes for better screen reader support.
<TabsList className="w-full justify-start border-b border-gray-300 bg-transparent p-0 h-auto rounded-none"> + <div role="tablist" aria-label={t("navigation_tabs")}> {navItems.map((item) => ( <Link key={item.path} href={item.path}> <TabsTrigger value={item.value} + role="tab" + aria-selected={currentTab === item.value} + aria-controls={`${item.value}-panel`} className="border-b-2 border-transparent px-2 py-2 text-gray-600 hover:text-gray-900 data-[state=active]:text-primary-800 data-[state=active]:border-primary-700 data-[state=active]:bg-transparent data-[state=active]:shadow-none rounded-none" > {item.title} </TabsTrigger> </Link> ))} + </div> </TabsList>src/pages/Facility/settings/organizations/FacilityOrganizationView.tsx (1)
46-52: Consider implementing debounce for search input.The search input directly updates the state without debouncing, which could trigger unnecessary API calls.
+import { useDebounce } from '@/hooks/useDebounce'; const [searchQuery, setSearchQuery] = useState(""); +const debouncedSearch = useDebounce(searchQuery, 300); -const filteredUsers = users?.results?.filter((userRole) => { +const filteredUsers = users?.results?.filter((userRole) => { - if (!searchQuery) return true; + if (!debouncedSearch) return true; - const searchLower = searchQuery.toLowerCase(); + const searchLower = debouncedSearch.toLowerCase(); const fullName = ...Also applies to: 53-63
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (1)
122-129: Consider adding lazy loading for avatars.For better performance, especially with many users, consider implementing lazy loading for avatar images.
<Avatar name={`${userRole.user.first_name} ${userRole.user.last_name}`} imageUrl={userRole.user.profile_picture_url} + loading="lazy" className="h-12 w-12 sm:h-14 sm:w-14 text-xl sm:text-2xl flex-shrink-0" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
public/locale/en.json(10 hunks)src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx(3 hunks)src/pages/Facility/settings/organizations/FacilityOrganizationView.tsx(5 hunks)src/pages/Facility/settings/organizations/components/CreateFacilityOrganizationSheet.tsx(7 hunks)src/pages/Facility/settings/organizations/components/FacilityOrganizationLayout.tsx(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
🔇 Additional comments (8)
src/pages/Facility/settings/organizations/components/CreateFacilityOrganizationSheet.tsx (1)
99-152: LGTM! Comprehensive internationalization implementation.All user-facing strings have been properly internationalized using the translation function. The implementation is consistent and follows best practices.
src/pages/Facility/settings/organizations/components/FacilityOrganizationLayout.tsx (1)
36-40: LGTM! Interface simplification.The
NavIteminterface has been simplified by replacing theiconproperty withvalue, making it more focused and maintainable.src/pages/Facility/settings/organizations/FacilityOrganizationView.tsx (1)
28-33: LGTM! Well-defined interface.The
Organizationinterface clearly defines the structure of organization objects with appropriate types and optional fields.public/locale/en.json (5)
293-293: LGTM! Department/Team related keys are well-structured.The keys for department/team management follow a consistent naming pattern and provide clear, user-friendly messages:
add_department_teamcreate_department_teamand its descriptionenter_department_team_nameand descriptionAlso applies to: 640-641, 934-935
1002-1005: LGTM! Organization type keys follow a consistent pattern.The organization type keys are well-organized with clear, descriptive values:
facility_organization_type__deptfacility_organization_type__otherfacility_organization_type__rootfacility_organization_type__team
648-648: LGTM! Organization creation keys are clear and consistent.The keys for organization creation provide appropriate feedback:
create_organizationorganization_created_successfullyAlso applies to: 1510-1510
1898-1898: LGTM! Search key is descriptive.The search key
search_by_department_team_nameclearly indicates its purpose.
1928-1928: LGTM! Action key is concise.The action key
see_detailsis simple and clear.
src/pages/Facility/settings/organizations/components/FacilityOrganizationLayout.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx
Outdated
Show resolved
Hide resolved
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
…ed pagination and search functionality
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (1)
65-74:⚠️ Potential issueOptimize search filtering logic.
The current implementation performs redundant client-side filtering after the server has already filtered the results based on the search query.
-const filteredUsers = users?.results?.filter((userRole) => { - if (!searchQuery) return true; - - const searchLower = searchQuery.toLowerCase(); - const fullName = - `${userRole.user.first_name} ${userRole.user.last_name}`.toLowerCase(); - const username = userRole.user.username.toLowerCase(); - - return fullName.includes(searchLower) || username.includes(searchLower); -}); +// Remove client-side filtering as search is already handled by the API +const filteredUsers = users?.results;
🧹 Nitpick comments (3)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (3)
37-39: Consider making the pagination limit configurable.The pagination limit is hardcoded to 12. Consider making it configurable through props or environment variables for better flexibility.
- const { qParams, Pagination, resultsPerPage } = useFilters({ - limit: 12, - }); + const { qParams, Pagination, resultsPerPage } = useFilters({ + limit: props.itemsPerPage ?? 12, + });
46-63: Enhance query configuration for better error handling and resilience.Consider adding error handling and retry configuration to improve the query's robustness.
const { data: users, isLoading: isLoadingUsers } = useQuery({ queryKey: [ "facilityOrganizationUsers", facilityId, id, searchQuery, qParams, ], queryFn: query.debounced(routes.facilityOrganization.listUsers, { pathParams: { facilityId, organizationId: id }, queryParams: { search: searchQuery || undefined, limit: resultsPerPage, offset: ((qParams.page || 1) - 1) * resultsPerPage, }, }), enabled: !!id, + retry: 3, + onError: (error) => { + console.error('Failed to fetch users:', error); + // Add error handling logic here + }, });
84-97: Enhance accessibility and error handling in the UI.Consider the following improvements:
- Add proper ARIA labels and roles for better accessibility
- Add error state handling in the UI
<div className="relative"> <CareIcon icon="l-search" className="absolute left-3 top-1/2 -translate-y-1/2 text-gray-500 h-4 w-4" + aria-hidden="true" /> <Input placeholder={t("search_by_user_name")} value={searchQuery} onChange={(e) => { setSearchQuery(e.target.value); }} className="w-full pl-8" + aria-label={t("search_by_user_name")} /> </div> {isLoadingUsers ? ( <div className="grid grid-cols-1 lg:grid-cols-2 xl:grid-cols-2 gap-4"> <CardGridSkeleton count={6} /> </div> +) : error ? ( + <Card className="col-span-full"> + <CardContent className="p-6 text-center text-red-500"> + {t("error_loading_users")} + </CardContent> + </Card> ) : ( <div className="space-y-4"> <div className="grid grid-cols-1 lg:grid-cols-2 xl:grid-cols-2 gap-4">Also applies to: 120-133
…mprove clarity and conditionally apply limits based on search query
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
public/locale/en.json (1)
2177-2177: Consider using a more specific key name for "type".The key "type" is very generic and could lead to confusion or conflicts. Consider using a more context-specific key name that clearly indicates its usage.
Example alternatives:
- "type": "Type", + "organization_type_label": "Type",or
- "type": "Type", + "department_type_label": "Type",depending on the context where it's used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(8 hunks)src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx
🔇 Additional comments (2)
public/locale/en.json (2)
293-294: Well-structured department/team localization keys!The new department/team related keys follow consistent naming conventions and provide clear, user-friendly messages.
Also applies to: 644-645, 945-946
653-653: Well-structured organization localization keys!The new organization related keys maintain consistency with existing patterns and provide clear feedback messages.
Also applies to: 1531-1531, 1659-1659, 1986-1986
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (2)
31-34: Consider enhancing type safety for sheet state.The
sheetproperty inSheetStatecould be more strictly typed using a union type of possible values.interface SheetState { - sheet: string; + sheet: "" | "add" | "link"; username: string; }
82-89: Enhance search input accessibility.Consider adding ARIA attributes and a label for better screen reader support.
<Input + aria-label={t("search_by_user_name")} placeholder={t("search_by_user_name")} value={searchQuery} onChange={(e) => { setSearchQuery(e.target.value); }} className="w-full pl-8" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#10568
File: src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx:69-78
Timestamp: 2025-02-13T19:11:07.106Z
Learning: Avoid implementing redundant client-side filtering when the API already supports search functionality, especially with paginated data, as it can lead to incorrect results and unnecessary performance overhead.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (2)
50-67: LGTM! Proper implementation of server-side search.The search implementation correctly utilizes the API's search functionality by passing the search query as a parameter, avoiding redundant client-side filtering.
155-165: Reconsider field truncation strategy.The current implementation truncates role and phone number fields, which could hide important information. Based on previous discussions, consider utilizing the available space better.
- <div className="font-medium truncate"> + <div className="font-medium break-words"> {userRole.role.name} </div> </div> <div> <div className="text-gray-500"> {t("phone_number")} </div> - <div className="font-medium truncate"> + <div className="font-medium break-words"> {userRole.user.phone_number} </div>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
public/locale/en.json (1)
1934-1934: Search Username Key Duplication
The key"search_by_user_name": "Search by user name"is nearly identical to"search_by_username": "Search by username"(found on the subsequent line). Please verify if both keys are needed or if they can be consolidated to avoid redundancy and potential confusion for translators.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
public/locale/en.json(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
🔇 Additional comments (7)
public/locale/en.json (7)
294-294: Add Department/Team Key Added
The new key"add_department_team": "Add Department/Team"is clear and consistent with similar action labels in the UI. Please confirm that this key is referenced correctly in the relevant components.
645-646: Create Department/Team & Description Keys
The keys"create_department_team": "Create Department/Team"and
"create_department_team_description": "Create a new department/team in this facility."
provide clear, actionable text for users. Ensure that these entries align with the overall tone and are properly integrated within the component that handles department/team creation.
1532-1532: Organization Creation Success Message
The entry"organization_created_successfully": "Organization created successfully"offers a concise success message. Verify that the corresponding component displays this message appropriately upon successful organization creation.
946-947: Department/Team Input Prompt Keys
The keys"enter_department_team_description": "Enter department/team description (optional)"and
"enter_department_team_name": "Enter department/team name"are clear and user-friendly, making them appropriate for form placeholder texts or labels. Please confirm that these texts consistently appear in the department/team creation UI.
1660-1660: Organization Name Prompt Key
The new entry"please_enter_organization_name": "Please enter an organization name"is straightforward and follows existing text patterns. It might be beneficial to review similar prompt messages elsewhere in the app to ensure uniformity in phrasing and capitalization.
1989-1989: Select Organization Type Prompt
The new key"select_organization_type": "Select organization type"is clearly worded for a dropdown prompt. Confirm that its usage context is consistent and that it aligns with similar keys throughout the project.
2180-2180: Generic 'Type' Key Consideration
The key"type": "Type"is very generic and may be used in many contexts. Although clear in isolation, please review its usage across the application to ensure that it does not introduce ambiguity. If it represents a specific field (e.g., organization type, resource type), consider a more descriptive key name.
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx
Outdated
Show resolved
Hide resolved
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (1)
67-81: Add accessibility improvements to the search input.While the search implementation is good, consider adding accessibility attributes:
<Input placeholder={t("search_by_user_name")} value={qParams.search || ""} onChange={(e) => { updateQuery({ search: e.target.value || undefined }); }} className="w-full pl-8" + aria-label={t("search_by_user_name")} + role="searchbox" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#10568
File: src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx:69-78
Timestamp: 2025-02-13T19:11:07.106Z
Learning: Avoid implementing redundant client-side filtering when the API already supports search functionality, especially with paginated data, as it can lead to incorrect results and unnecessary performance overhead.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (3)
32-41: LGTM! Clean state management implementation.The state management implementation is well-structured with:
- Clear separation between sheet state and filter parameters
- Proper integration of useFilters hook for pagination
- Removal of redundant client-side filtering as discussed in previous reviews
47-58: LGTM! Well-implemented API integration.The query setup properly handles:
- Search functionality through API parameters
- Pagination with correct offset calculation
- Debounced query execution
146-156: Replace truncation with a more user-friendly approach.As discussed in previous comments, truncating role and phone number fields may not provide the best user experience. Consider using the available white space more effectively.
- <div className="font-medium truncate"> + <div className="font-medium break-words"> {userRole.role.name} </div> </div> <div> <div className="text-gray-500"> {t("phone_number")} </div> - <div className="font-medium truncate"> + <div className="font-medium break-words"> {userRole.user.phone_number} </div>
rithviknishad
added
tested
reviewed
|
@abhimanyurajeesh 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! 🙌 |
Required Backend
Proposed Changes
Current Design
Requested Design
Changed Design
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes