Enhanced Resource Page #9768
Enhanced Resource Page #9768
Conversation
WalkthroughThis pull request introduces a comprehensive redesign of the resource management interface. The changes include removing the separate 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. |
|
@rithviknishad review this one |
![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 (4)
src/components/Kanban/Board.tsx (1)
38-39: Removed horizontal scrolling buttons and simplified layout.The new two-tier container approach (outer
+ flex boxes) is neat. Ensure no accessibility regression for users who relied on the previous scroll control.src/components/Resource/ResourceCardList.tsx (1)
26-85: Card layout effectively conveys resource info.Using a grid and separate display for title, status, and emergency badge is clear, providing a well-organized view. Ensure each field is accessible with descriptive ARIA labels if needed.
src/components/Resource/ResourcePage.tsx (1)
94-126: Consider removing redundant fragments.A fragment wrapping a single child is flagged by the linter. You can safely remove it to reduce JSX verbosity.
- <> <div className="flex lg:flex-row flex-col w-full justify-end gap-2"> - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 94-126: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
public/locale/en.json (1)
1616-1616: LGTM! Consider adding a placeholder variant.The new translation key follows the naming convention and is properly placed. For consistency with other selection fields in the application, consider adding a placeholder variant:
"select_status": "Select Status", +"select_status_placeholder": "Select a status",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
public/locale/en.json(1 hunks)src/Routers/routes/ResourceRoutes.tsx(1 hunks)src/components/Kanban/Board.tsx(3 hunks)src/components/Resource/ResourceBoard.tsx(0 hunks)src/components/Resource/ResourceCardList.tsx(1 hunks)src/components/Resource/ResourceList.tsx(0 hunks)src/components/Resource/ResourcePage.tsx(1 hunks)
💤 Files with no reviewable changes (2)
- src/components/Resource/ResourceList.tsx
- src/components/Resource/ResourceBoard.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
src/components/Resource/ResourcePage.tsx
[error] 94-126: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (16)
src/Routers/routes/ResourceRoutes.tsx (2)
3-3: Successfully imported the newResourcePagecomponent.The import statement is clear and aligns with the consolidated view approach.
8-8: Route consolidation looks good.Replacing the old
BoardView/ListViewroutes withResourcePageis consistent with the PR objective of unifying the resource views.src/components/Kanban/Board.tsx (2)
137-137: Height change fromcalc(100vh-200px)tocalc(100vh-340px).Increasing the offset reduces visible space, which might cause more scrolling. Verify on smaller devices to ensure no usability issues.
169-169: Placeholder usage in Droppable context.Including
provided.placeholderensures proper rendering of dragged items. This is currently a best practice for React DnD.src/components/Resource/ResourceCardList.tsx (5)
1-3: Imports are well-structured.Imports for
LinkanduseTranslationare minimal and aligned with usage below.
11-13: Interface definition is clear.
ResourceCardListPropsis concise, making the component’s expected data structure explicit.
18-24: Graceful handling of empty data.Rendering "no_results_found" message improves user feedback when there are no resources.
131-135: “View all details” link for each resource.This link offers a direct route to resource details, improving discoverability.
142-142: Export statement confirmed.Exporting
ResourceCardListas default matches the new consolidated design approach.src/components/Resource/ResourcePage.tsx (7)
42-45: Lazy loading the KanbanBoard component.This reduces initial bundle size and is a strong performance optimization for routes that may not require the board immediately.
51-67: ResourcePage initialization logic.Storing filter states (
boardFilter,viewMode) is straightforward. The fallback to default active statuses plus an option for completed items is well-structured.
68-77: Fetch logic usinguseTanStackQueryInstead.Looks correct. The query configuration matches the advanced filter structure. Confirm that all relevant data fields are included in the final payload for the Resource list.
79-84: Export function for CSV.Nice use of
requestwithcsv: true. This helps quickly integrate a CSV export feature.
129-227: Board view integration.The board filter and drag-and-drop updates to resource status align well with the consolidated view. Watch out for edge cases when switching between active and completed boards.
228-297: List view integration with advanced filters.Combining the
ResourceCardList, refresh actions, and pagination is well-organized. The consistent approach to filtering across both views helps unify UX.
302-302: ExportingResourcePage.This final export aligns with the route changes in
ResourceRoutes.tsx, ensuring navigational consistency.
|
I am unable to reopen that PR as I've reforked the repo 😅 |
|
solve the changes requested in previous PR here atleast then? |
I think I've done that🤔Let me list the changes.
|
|
let's keep the implementation idential to the one in appointments, and delete that kanban board |
got it |
Jacobjeevan
added
changes required
and removed
question
@rithviknishad Do we need DragDropContext in resource page board view? |
…enhanced-resource-page
…enhanced-resource-page
…enhanced-resource-page
…enhanced-resource-page
|
@rithviknishad , I’ve tried implementing this in the same way as the AppointmentPage. Is this correct? |
| const appliedFilters = formatFilter(qParams); | ||
| const [viewMode, setViewMode] = useState<"board" | "list">("board"); | ||
| const { exportFile } = useExport(); | ||
| const { loading, data, refetch } = useTanStackQueryInstead( |
There was a problem hiding this comment.
Switch to using useQuery + query here.
Do the same for other API calls in this component 👍
| }); | ||
| return data as QueryResponse<T>; | ||
| } catch (error) { | ||
| console.error("Error fetching section data:", error); |
There was a problem hiding this comment.
Remove the console log
| const options = section.fetchOptions(section.id); | ||
| const fetchPage = async ({ pageParam = 0 }) => { | ||
| try { | ||
| const data = await callApi(options.route, { |
There was a problem hiding this comment.
useQuery instead of callApi.
| const [boardFilter, setBoardFilter] = useState(ACTIVE); | ||
| // eslint-disable-next-line | ||
| const appliedFilters = formatFilter(qParams); | ||
| const [viewMode, setViewMode] = useState<"board" | "list">("board"); |
There was a problem hiding this comment.
You should add the logic for storing in localStorage for viewMode, right now it resets on every page load.
We can create a hook to handle that in a different issue, since we will have multiple components having a similar view.
There was a problem hiding this comment.
@Jacobjeevan for now I've use useEffect, should I create a new issue for the hook itself? 🤔
…enhanced-resource-page
…enhanced-resource-page
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/components/Resource/ResourcePage.tsx (2)
300-325: Optimize performance with React.memo.Consider memoizing the ResourceColumn component to prevent unnecessary re-renders when parent components update.
-function ResourceColumn<T extends { id: string }>( +const ResourceColumn = React.memo(<T extends { id: string }>( props: ResourceColumnProps<T>, ) { const board = useRef<HTMLDivElement>(null); return ( // ... component implementation ); -} +});
480-593: Improve type safety and styling maintainability.Consider the following improvements:
- Use proper type checking for the resource parameter
- Extract repeated CSS classes into reusable Tailwind components
- return data.map((resource: ResourceRequest, i) => ( + return data.map((resource, i) => ( <div key={i} - className="w-full border border-b-2 border-gray-200 col-span-6" + className={clsx( + "w-full border border-b-2 border-gray-200 col-span-6", + "resource-row" + )} >Consider creating a CSS module or Tailwind components for commonly used class combinations to improve maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Kanban/Board.tsx(0 hunks)src/components/Resource/ResourcePage.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/Kanban/Board.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
| <Select | ||
| defaultValue="active" | ||
| onValueChange={(value) => | ||
| setBoardFilter(value === "completed" ? COMPLETED : ACTIVE) | ||
| } | ||
| > | ||
| <SelectTrigger className="w-full lg:w-[180px]"> | ||
| <SelectValue defaultValue={"completed"} /> | ||
| </SelectTrigger> | ||
| <SelectContent> | ||
| <SelectGroup> | ||
| <SelectItem value="active">{t("active")}</SelectItem> | ||
| <SelectItem value="completed">{t("completed")}</SelectItem> | ||
| </SelectGroup> | ||
| </SelectContent> | ||
| </Select> |
There was a problem hiding this comment.
Fix inconsistent default values in Select component.
The defaultValue prop in SelectTrigger and the defaultValue in SelectValue are inconsistent. The SelectTrigger uses "active" while SelectValue uses "completed".
<Select
defaultValue="active"
onValueChange={(value) =>
setBoardFilter(value === "completed" ? COMPLETED : ACTIVE)
}
>
<SelectTrigger className="w-full lg:w-[180px]">
- <SelectValue defaultValue={"completed"} />
+ <SelectValue defaultValue={"active"} />
</SelectTrigger>
<SelectContent>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Select | |
| defaultValue="active" | |
| onValueChange={(value) => | |
| setBoardFilter(value === "completed" ? COMPLETED : ACTIVE) | |
| } | |
| > | |
| <SelectTrigger className="w-full lg:w-[180px]"> | |
| <SelectValue defaultValue={"completed"} /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| <SelectGroup> | |
| <SelectItem value="active">{t("active")}</SelectItem> | |
| <SelectItem value="completed">{t("completed")}</SelectItem> | |
| </SelectGroup> | |
| </SelectContent> | |
| </Select> | |
| <Select | |
| defaultValue="active" | |
| onValueChange={(value) => | |
| setBoardFilter(value === "completed" ? COMPLETED : ACTIVE) | |
| } | |
| > | |
| <SelectTrigger className="w-full lg:w-[180px]"> | |
| <SelectValue defaultValue={"active"} /> | |
| </SelectTrigger> | |
| <SelectContent> | |
| <SelectGroup> | |
| <SelectItem value="active">{t("active")}</SelectItem> | |
| <SelectItem value="completed">{t("completed")}</SelectItem> | |
| </SelectGroup> | |
| </SelectContent> | |
| </Select> |
| const fetchPage = async ({ pageParam = 0 }) => { | ||
| try { | ||
| const data = await query(options.route, { | ||
| queryParams: { | ||
| ...options.options?.query, | ||
| offset: pageParam, | ||
| limit: defaultLimit, | ||
| }, | ||
| })({ signal: new AbortController().signal }); | ||
| return data as QueryResponse<T>; | ||
| } catch (error) { | ||
| return { results: [], next: null, count: 0 }; | ||
| } | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and request cancellation.
The current implementation has two issues:
- The AbortController is created but never stored or used effectively
- The error handling silently fails without providing useful information
const fetchPage = async ({ pageParam = 0 }) => {
+ const controller = new AbortController();
try {
const data = await query(options.route, {
queryParams: {
...options.options?.query,
offset: pageParam,
limit: defaultLimit,
},
- })({ signal: new AbortController().signal });
+ })({ signal: controller.signal });
return data as QueryResponse<T>;
} catch (error) {
+ if (error instanceof Error) {
+ console.error(`Failed to fetch page: ${error.message}`);
+ }
return { results: [], next: null, count: 0 };
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const fetchPage = async ({ pageParam = 0 }) => { | |
| try { | |
| const data = await query(options.route, { | |
| queryParams: { | |
| ...options.options?.query, | |
| offset: pageParam, | |
| limit: defaultLimit, | |
| }, | |
| })({ signal: new AbortController().signal }); | |
| return data as QueryResponse<T>; | |
| } catch (error) { | |
| return { results: [], next: null, count: 0 }; | |
| } | |
| }; | |
| const fetchPage = async ({ pageParam = 0 }) => { | |
| const controller = new AbortController(); | |
| try { | |
| const data = await query(options.route, { | |
| queryParams: { | |
| ...options.options?.query, | |
| offset: pageParam, | |
| limit: defaultLimit, | |
| }, | |
| })({ signal: controller.signal }); | |
| return data as QueryResponse<T>; | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| console.error(`Failed to fetch page: ${error.message}`); | |
| } | |
| return { results: [], next: null, count: 0 }; | |
| } | |
| }; |
| const exportAction = async () => { | ||
| const { data } = await request(routes.downloadResourceRequests, { | ||
| query: { ...appliedFilters, csv: true }, | ||
| }); | ||
| return data ?? null; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to exportAction function.
The function should handle potential network errors and invalid responses explicitly.
const exportAction = async () => {
+ try {
const { data } = await request(routes.downloadResourceRequests, {
query: { ...appliedFilters, csv: true },
});
return data ?? null;
+ } catch (error) {
+ console.error('Failed to export resource requests:', error);
+ throw new Error(t('failed_to_export'));
+ }
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const exportAction = async () => { | |
| const { data } = await request(routes.downloadResourceRequests, { | |
| query: { ...appliedFilters, csv: true }, | |
| }); | |
| return data ?? null; | |
| }; | |
| const exportAction = async () => { | |
| try { | |
| const { data } = await request(routes.downloadResourceRequests, { | |
| query: { ...appliedFilters, csv: true }, | |
| }); | |
| return data ?? null; | |
| } catch (error) { | |
| console.error('Failed to export resource requests:', error); | |
| throw new Error(t('failed_to_export')); | |
| } | |
| }; |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
Release Notes
New Features
Localization
UI/UX Changes
Removed Components