Resources redesign #10308
Resources redesign #10308
Conversation
WalkthroughThis pull request introduces comprehensive changes to the resources management system, focusing on localization, routing, and component restructuring. The modifications span multiple files, including locale JSON files, routing configurations, and React components. The changes aim to enhance the resource management interface by updating status representations, improving localization support, and refactoring the resource list and related components. Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (24)
💤 Files with no reviewable changes (24)
⏰ Context from checks skipped due to timeout of 90000ms (4)
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: 0
🧹 Nitpick comments (6)
src/components/Resource/ResourceList.tsx (5)
2-2: Consider using theuseTranslationhook for better context alignment.
Directly importingtfrom"i18next"can work, but using theuseTranslationhook fromreact-i18nextoften provides more robust context management and can help avoid unexpected behavior if multiple i18n instances are in use.
42-45: Avoid basing logic on display text.
COMPLETEDandACTIVEare derived by filtering thetextfield ofRESOURCE_STATUS_CHOICES. Depending on translations or display text for logic can be fragile. Consider using IDs or status codes instead.
85-97: Add explicit error handling for the query.
While theisLoadingstate is handled, consider adding anonErrorcallback or conditional rendering for errors to improve resiliency and user feedback when requests fail.
200-221: Reuse existing translation keys if applicable.
Based on prior learnings from AnveshNalimela, there's a recommendation to unify status translations under certain keys (e.g."consent__status"). Verify whether reusing those keys (instead of"resource_status__${statusOption}") aligns with your global i18n strategy.
228-299: Consider splitting the rendering block into smaller components.
The mapping logic for resources, empty-state handling, and pagination result in a large JSX block. Refactoring into smaller subcomponents could improve readability and maintainability.src/components/Resource/ResourceDetailsUpdate.tsx (1)
206-208: Confirm accurate translation keys for resource statuses.
optionLabel={(option) => t(\resource_status__${option}`)}` may require matching keys in your localization files. Use a stable ID/key rather than a user-facing string if your translations or statuses diverge in the future. Also consider adopting past recommendations from retrieved learnings about unifying status translation keys.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
public/locale/en.json(5 hunks)public/locale/hi.json(0 hunks)public/locale/kn.json(0 hunks)public/locale/ml.json(0 hunks)public/locale/ta.json(0 hunks)src/Routers/routes/ResourceRoutes.tsx(1 hunks)src/common/constants.tsx(1 hunks)src/components/Resource/ResourceBadges.tsx(0 hunks)src/components/Resource/ResourceBlock.tsx(0 hunks)src/components/Resource/ResourceBoard.tsx(0 hunks)src/components/Resource/ResourceCommons.tsx(0 hunks)src/components/Resource/ResourceDetailsUpdate.tsx(2 hunks)src/components/Resource/ResourceFilter.tsx(0 hunks)src/components/Resource/ResourceList.tsx(1 hunks)
💤 Files with no reviewable changes (9)
- src/components/Resource/ResourceBoard.tsx
- src/components/Resource/ResourceBadges.tsx
- public/locale/ta.json
- src/components/Resource/ResourceBlock.tsx
- src/components/Resource/ResourceCommons.tsx
- src/components/Resource/ResourceFilter.tsx
- public/locale/ml.json
- public/locale/hi.json
- public/locale/kn.json
🧰 Additional context used
📓 Learnings (1)
src/components/Resource/ResourceDetailsUpdate.tsx (1)
Learnt from: AnveshNalimela
PR: ohcnetwork/care_fe#9096
File: src/components/Resource/ResourceList.tsx:245-245
Timestamp: 2024-11-13T12:33:52.373Z
Learning: When displaying the resource status in `ResourceList.tsx`, use `{t("consent__status")}` instead of adding a new translation key `{t("resource_status")}`.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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: CodeQL-Build
- GitHub Check: OSSAR-Scan
🔇 Additional comments (5)
src/components/Resource/ResourceList.tsx (1)
47-59: Nice UX improvement with the newEmptyStatecomponent!
This is a clean, self-contained implementation for an empty-state view. It helps users easily see when no resources exist.src/Routers/routes/ResourceRoutes.tsx (1)
3-8: Clean route definition transition.
Switching from<View>to<ResourceList>is straightforward and appears consistent with the new design. Double-check that all references toListViewhave been removed in case they are still called in other files.✅ Verification successful
Clean transition confirmed - no remaining ListView references
The codebase shows a complete transition toResourceListwith no remaining references to the oldListVieworViewcomponents.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find any references to the old `ListView` or `View` usage in the repo. rg -A 2 "ListView" rg -A 2 "View name=\"resource\""Length of output: 51
Script:
#!/bin/bash # First verify we can access the codebase ls src/Routers/routes/ResourceRoutes.tsx # Search for any View-related components with broader patterns rg -i "view" src/ rg "from.*View" src/ rg "import.*View" src/ # Search for potential resource-related view components rg -i "resource.*view" src/ rg -i "view.*resource" src/Length of output: 26090
src/common/constants.tsx (1)
165-173: Well-structured resource status mapping with semantic icons!The constant provides a clear mapping between status values and their visual representations, with semantically meaningful icons that enhance UX.
public/locale/en.json (2)
1734-1740: Comprehensive status translations with proper formatting!The translations for resource statuses are well-structured and maintain consistency with the status values defined in the constants.
326-326: Clear empty state messages for better UX!The empty state messages provide clear guidance to users when no resources are found and suggest next actions.
Also applies to: 1367-1367
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Resource/ResourceList.tsx (1)
265-359: Consider adding error boundary for the resource list.The card grid implementation looks good, but it could benefit from error handling for failed resource loads.
+import { ErrorBoundary } from "@/components/ErrorBoundary"; <div className="grid gap-4 sm:grid-cols-1 md:grid-cols-2 lg:grid-cols-3" data-cy="resource-list-cards" > + <ErrorBoundary> {isLoading ? ( <CardGridSkeleton count={6} /> ) : resources.length === 0 ? ( <div className="col-span-full"> <EmptyState /> </div> ) : ( // ... rest of the code )} + </ErrorBoundary> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
public/locale/en.json(7 hunks)src/Routers/routes/ResourceRoutes.tsx(1 hunks)src/components/Patient/PatientDetailsTab/ResourceRequests.tsx(1 hunks)src/components/Resource/ResourceCreate.tsx(1 hunks)src/components/Resource/ResourceDetails.tsx(5 hunks)src/components/Resource/ResourceDetailsUpdate.tsx(5 hunks)src/components/Resource/ResourceList.tsx(1 hunks)src/components/ui/sidebar/facility-nav.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Resource/ResourceDetailsUpdate.tsx
🧰 Additional context used
📓 Learnings (2)
src/components/Resource/ResourceCreate.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9062
File: src/Routers/routes/ResourceRoutes.tsx:19-21
Timestamp: 2024-12-10T02:49:05.600Z
Learning: In the `ResourceCreate` component, `facilityId` can be optional.
src/Routers/routes/ResourceRoutes.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9062
File: src/Routers/routes/ResourceRoutes.tsx:19-21
Timestamp: 2024-12-10T02:49:05.600Z
Learning: In the `ResourceCreate` component, `facilityId` can be optional.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (11)
src/components/ui/sidebar/facility-nav.tsx (1)
38-42: LGTM!The change correctly aligns the resource URL with other facility-scoped links by incorporating the
baseUrl. This ensures proper facility context in resource navigation.src/components/Patient/PatientDetailsTab/ResourceRequests.tsx (1)
115-119: LGTM!The change correctly updates the resource detail URL to include the origin facility's ID, maintaining proper facility context in navigation.
src/components/Resource/ResourceDetails.tsx (2)
206-212: LGTM!The component's props have been correctly updated to include the required
facilityId, aligning with the facility-scoped resource redesign.
272-274: Improved localization for resource status.The status badge now uses a translation key format that better supports localization.
src/components/Resource/ResourceCreate.tsx (2)
133-133: LGTM!The change correctly updates the post-creation navigation to include the facility ID, maintaining proper facility context.
Line range hint
47-49: Consider making facilityId optional.According to the retrieved learnings from PR #9062,
facilityIdcan be optional in theResourceCreatecomponent. However, the current interface declares it as a required number.✅ Verification successful
Make
facilityIdoptional and change type to string for consistencyThe suggestion to make
facilityIdoptional is valid and should be implemented along with changing its type fromnumbertostringfor consistency with other components. The code already handles the optional case in the facility query withenabled: !!facilityId.
- Change the interface to use
facilityId?: string- Remove
String()conversions as they won't be needed anymore🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if facilityId is optional in other components # Test: Search for components that make facilityId optional rg -A 5 'facilityId\?' src/Length of output: 5894
Script:
#!/bin/bash # Check interface definition and usage of facilityId in ResourceCreate ast-grep --pattern 'interface $_Props { $$$ facilityId: $_ $$$ }' src/components/Resource/ResourceCreate.tsx echo "=== Usage patterns ===" rg -A 3 'facilityId' src/components/Resource/ResourceCreate.tsxLength of output: 1316
src/Routers/routes/ResourceRoutes.tsx (1)
8-16: LGTM! Routes are properly scoped to facilities.The route changes correctly implement facility-scoped resource management by:
- Including
facilityIdparameter in all resource routes- Passing
facilityIdprop to all resource componentsNote: Based on past learnings from PR #9062,
facilityIdcan be optional in theResourceCreatecomponent, but it's correctly required here in the routes where facility context is needed.src/components/Resource/ResourceList.tsx (3)
1-46: LGTM! Well-structured imports and constants.The imports are properly organized by type (external libraries, utils, components) and the constants for status management are clearly defined.
48-60: LGTM! EmptyState component follows best practices.The
EmptyStatecomponent is well-implemented with:
- Clear visual hierarchy
- Proper use of icons and spacing
- Internationalized text
62-101: LGTM! Data fetching implementation is robust.The component properly:
- Uses React Query for efficient data fetching
- Handles pagination
- Manages filter state
- Scopes requests to the facility
public/locale/en.json (1)
326-326: LGTM! Comprehensive localization coverage.The added localization strings provide complete coverage for:
- Resource statuses
- Search functionality
- Empty states
- Incoming/Outgoing filters
Also applies to: 1075-1075, 1368-1368, 1443-1443, 1736-1743
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
Localization
Resource Management
UI/UX Improvements
Code Refactoring