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
Sequence Diagram(s)sequenceDiagram
participant User
participant UI
participant API
User->>UI: Enter search query
UI->>UI: Update search state (`searchQuery`)
UI->>API: Fetch users with query parameter (search)
API-->>UI: Return filtered users data
UI->>User: Render updated user list with skeleton loader until data loads
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 (1)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (1)
151-153: Consider alternative layout for truncated fields.Based on the previous discussion, the truncation of role and phone number fields might not be the best approach. Consider:
- Utilizing the available white space horizontally
- Adjusting the layout for mobile view to prevent truncation
- <div className="font-medium truncate"> + <div className="font-medium break-words"> {userRole.role.name} </div>- <div className="font-medium truncate"> + <div className="font-medium break-words"> {userRole.user.phone_number} </div>Also applies to: 159-161
📜 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 (5)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (4)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (4)
3-3: LGTM! Well-structured component setup.The component setup is clean and follows React best practices. The imports are appropriate, and the filter configuration with a default limit of 12 items per page is reasonable for this use case.
Also applies to: 10-10, 13-13, 16-16, 37-40
46-63: LGTM! Proper implementation of server-side search.The query configuration correctly implements server-side search by:
- Including searchQuery in the queryKey for proper cache invalidation
- Using search_text parameter for server-side filtering
- Implementing proper pagination parameters
72-86: LGTM! Well-implemented search UI with proper accessibility.The search input implementation is clean with:
- Proper icon positioning
- Responsive layout
- Internationalization support
- Accessible input field
185-191: LGTM! Proper pagination implementation.The pagination is correctly implemented with:
- Proper conditional rendering
- Appropriate total count handling
- Integration with the useFilters hook
its fix by changing the layout size. |
…heet components for improved responsiveness and consistency
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx
Outdated
Show resolved
Hide resolved
…se local state instead of query parameters for sheet handling
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (1)
2-2: Remove commented import.The commented import
useQueryParamsshould be removed if it's no longer needed.-//import { useQueryParams } from "raviger";
📜 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: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (4)
src/pages/Facility/settings/organizations/FacilityOrganizationUsers.tsx (4)
51-68: LGTM! Proper implementation of server-side search.The search implementation correctly utilizes the backend API by passing the search query as a parameter, avoiding redundant client-side filtering. This is the right approach for paginated data.
77-91: LGTM! Well-implemented search UI with proper accessibility.The search input implementation includes:
- Clear visual hierarchy with the search icon
- Proper responsive layout
- Accessible input with placeholder text
156-166: Reconsider field truncation approach.Based on previous discussions, truncating role and phone number fields may not be the best approach. Consider utilizing the available horizontal space more effectively.
Apply this diff to improve field visibility:
- <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>
190-196: LGTM! Well-implemented pagination.The pagination implementation correctly:
- Checks for results existence
- Verifies count threshold
- Centers the pagination controls
Required Backend
Proposed Changes
Current Design
Requested Design
Changed Design
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes