Enhanced Resource Page #9768
Enhanced Resource Page #9768
Conversation
WalkthroughThis pull request introduces changes to the resource management interface by adding a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
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? 🤔
There was a problem hiding this comment.
Nah, @Mahendar0701 is taking care of that in another issue.
There was a problem hiding this comment.
Nah, @Mahendar0701 is taking care of that in another issue.
@Jacobjeevan okay, should I remove my implementation then or keep it as it is?🤔
|
@rithviknishad @Jacobjeevan any update on this? 😅 |
| SelectTrigger, | ||
| SelectValue, | ||
| } from "../ui/select"; | ||
| import { Tabs, TabsList, TabsTrigger } from "../ui/tabs"; |
There was a problem hiding this comment.
minor thing, but absolute imports
| 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.
Nah, @Mahendar0701 is taking care of that in another issue.
| <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.
@manmeetnagii ☝️ Also you don't need to specify defaultValue in both places.
Jacobjeevan
removed
question
…o enhanced-resource-page
| variant="secondary" | ||
| className=" bg-transparent shadow-none text-black rounded-full" | ||
| action={async () => { | ||
| const { data } = await request( |
There was a problem hiding this comment.
There's still this request :)
…enhanced-resource-page
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/components/Resource/ResourcePage.tsx (3)
1-18: Convert relative imports to absolute imports.Based on the previous review comments, the codebase follows a convention of using absolute imports.
-import { useTranslation } from "react-i18next"; +import { useTranslation } from "@/i18n";
435-453: Replace 'any' type with proper typing.Using 'any' type reduces type safety. Consider using the generic type parameter T.
-{items.map((item: any, index) => { +{items.map((item: T, index) => {
486-486: Simplify grid classes using Tailwind @apply.Consider extracting repeated grid classes into a custom Tailwind class to follow DRY principles.
-<div className=" flex grid w-full gap-1 overflow-hidden bg-white p-4 sm:grid-cols-1 md:grid-cols-1 lg:grid-cols-5"> +<div className="resource-grid-layout">Add to your Tailwind CSS file:
@layer components { .resource-grid-layout { @apply flex grid w-full gap-1 overflow-hidden bg-white p-4 sm:grid-cols-1 md:grid-cols-1 lg:grid-cols-5; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(3 hunks)src/components/Resource/ResourcePage.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- public/locale/en.json
🧰 Additional context used
📓 Learnings (1)
src/components/Resource/ResourcePage.tsx (1)
Learnt from: Jacobjeevan
PR: ohcnetwork/care_fe#9768
File: src/components/Resource/ResourcePage.tsx:80-89
Timestamp: 2025-01-20T09:11:54.138Z
Learning: In the care_fe project, useQuery and useMutation hooks have global error handlers. Additional error handlers should only be added when specific functionality is needed beyond the global handler. In such cases, prefer using built-in params like isSuccess and isError from useQuery/useMutation.
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (2)
src/components/Resource/ResourcePage.tsx (2)
96-106: LGTM! View mode persistence implementation.The implementation correctly persists and restores the view mode preference using localStorage, as discussed in previous review comments.
278-317: Well-structured generic component implementation!The ResourceColumn component is well-implemented with proper type safety and clean separation of concerns.
| const exportAction = () => { | ||
| const data = query(routes.downloadResourceRequests, { | ||
| queryParams: { ...appliedFilters, csv: true }, | ||
| })({ signal: new AbortController().signal }); | ||
| return data ?? null; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve request cancellation in exportAction.
The AbortController is created but never stored, making it impossible to cancel the request if needed.
const exportAction = () => {
+ const controller = new AbortController();
const data = query(routes.downloadResourceRequests, {
queryParams: { ...appliedFilters, csv: true },
- })({ signal: new AbortController().signal });
+ })({ signal: controller.signal });
return data ?? null;
};📝 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 = () => { | |
| const data = query(routes.downloadResourceRequests, { | |
| queryParams: { ...appliedFilters, csv: true }, | |
| })({ signal: new AbortController().signal }); | |
| return data ?? null; | |
| }; | |
| const exportAction = () => { | |
| const controller = new AbortController(); | |
| const data = query(routes.downloadResourceRequests, { | |
| queryParams: { ...appliedFilters, csv: true }, | |
| })({ signal: controller.signal }); | |
| return data ?? null; | |
| }; |
| <ExportButton | ||
| variant="secondary" | ||
| className=" bg-transparent shadow-none text-black rounded-full" | ||
| action={async () => { | ||
| const data = query(routes.downloadResourceRequests, { | ||
| queryParams: { | ||
| ...formatFilter({ ...qParams, status: section.id }), | ||
| csv: true, | ||
| }, | ||
| })({ signal: new AbortController().signal }); | ||
| return data ?? null; | ||
| }} | ||
| filenamePrefix={`resource_requests_${section.id}`} | ||
| /> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix request cancellation in ExportButton action.
Similar to the main exportAction, the AbortController is created but never stored.
action={async () => {
+ const controller = new AbortController();
const data = query(routes.downloadResourceRequests, {
queryParams: {
...formatFilter({ ...qParams, status: section.id }),
csv: true,
},
- })({ signal: new AbortController().signal });
+ })({ signal: controller.signal });
return data ?? null;
}}📝 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.
| <ExportButton | |
| variant="secondary" | |
| className=" bg-transparent shadow-none text-black rounded-full" | |
| action={async () => { | |
| const data = query(routes.downloadResourceRequests, { | |
| queryParams: { | |
| ...formatFilter({ ...qParams, status: section.id }), | |
| csv: true, | |
| }, | |
| })({ signal: new AbortController().signal }); | |
| return data ?? null; | |
| }} | |
| filenamePrefix={`resource_requests_${section.id}`} | |
| /> | |
| </div> | |
| <ExportButton | |
| variant="secondary" | |
| className=" bg-transparent shadow-none text-black rounded-full" | |
| action={async () => { | |
| const controller = new AbortController(); | |
| const data = query(routes.downloadResourceRequests, { | |
| queryParams: { | |
| ...formatFilter({ ...qParams, status: section.id }), | |
| csv: true, | |
| }, | |
| })({ signal: controller.signal }); | |
| return data ?? null; | |
| }} | |
| filenamePrefix={`resource_requests_${section.id}`} | |
| /> | |
| </div> |
…enhanced-resource-page
@Jacobjeevan Done 😅 |
|
Resources pages are going to be rebuilt anyways soon |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Summary by CodeRabbit
Release Notes
New Features
Localization
Refactor
Improvements