Redesign: Departments page root view #10411
Conversation
WalkthroughThis pull request introduces enhancements to both the localization data and the facility organization view. The localization file has new keys added to support enhanced user prompts and actions. The organization component has been refactored from a card-based layout to an expandable table layout, featuring state management for row expansion, toggle functionality, and a filtering input for organization names. These changes align with the redesign requirements for the facility department section. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Index as FacilityOrganizationIndex
participant Row as OrganizationRow
User->>Index: Clicks on a row
Index->>Index: toggleRow(rowId) updates expandedRows
Index->>Index: getChildren(parentId) retrieves child organizations
Index->>Row: Renders expanded child rows
User->>Index: Enters filter text
Index->>User: Displays filtered organizations
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip 🌐 Web search-backed reviews and chat
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: 1
🧹 Nitpick comments (2)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx (2)
93-98: Consider memoizing helper functions for better performance.The
getChildrenfunction could benefit from memoization usinguseMemoto prevent unnecessary filtering on each render, especially with large datasets.-const getChildren = (parentId: string) => { +const getChildren = useMemo(() => (parentId: string) => { return data.results.filter((org) => org.parent?.id === parentId); -}; +}, [data.results]);
99-236: Clean up unnecessary Fragment wrapper.The Fragment wrapper around the buttons is redundant as it contains only one child element.
- <> {children.length > 0 ? ( <> <Button>...</Button> <Button>...</Button> </> ) : ( <Button>...</Button> )} - </>Well-implemented recursive component structure!
The OrganizationRow component effectively handles the hierarchical display of organizations with proper TypeScript types and clean recursion.
🧰 Tools
🪛 Biome (1.9.4)
[error] 173-216: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(6 hunks)src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx
[error] 173-216: 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)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx (1)
1-48: Well-structured state management for the table layout!The addition of expandedRows state and table-related imports aligns well with the redesign from cards to an expandable table layout.
public/locale/en.json (1)
293-294: Well-structured localization keys!The new translation keys are properly organized and follow the established naming conventions:
- add_department_or_team
- click
- expand_all
- filter_by_department_or_team_name
- to_create_a_new_one
- to_open_manage_users_or_create_more_departments_teams_within_it
Also applies to: 565-566, 948-949, 1019-1020, 2035-2036
| <div className="w-60"> | ||
| <Input | ||
| className="px-2 placeholder:text-xs placeholder:text-[#4B5563]" | ||
| placeholder={t("filter_by_department_or_team_name")} | ||
| ></Input> | ||
| </div> |
There was a problem hiding this comment.
Implement the filter functionality for the search input.
The search input is present but lacks an onChange handler to filter the organizations.
<Input
className="px-2 placeholder:text-xs placeholder:text-[#4B5563]"
placeholder={t("filter_by_department_or_team_name")}
+ onChange={(e) => {
+ const searchTerm = e.target.value.toLowerCase();
+ setFilteredResults(
+ data.results.filter(org =>
+ org.name.toLowerCase().includes(searchTerm)
+ )
+ );
+ }}
></Input>📝 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.
| <div className="w-60"> | |
| <Input | |
| className="px-2 placeholder:text-xs placeholder:text-[#4B5563]" | |
| placeholder={t("filter_by_department_or_team_name")} | |
| ></Input> | |
| </div> | |
| <div className="w-60"> | |
| <Input | |
| className="px-2 placeholder:text-xs placeholder:text-[#4B5563]" | |
| placeholder={t("filter_by_department_or_team_name")} | |
| onChange={(e) => { | |
| const searchTerm = e.target.value.toLowerCase(); | |
| setFilteredResults( | |
| data.results.filter(org => | |
| org.name.toLowerCase().includes(searchTerm) | |
| ) | |
| ); | |
| }} | |
| ></Input> | |
| </div> |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx (1)
248-253:⚠️ Potential issueImplement the filter functionality for the search input.
The search input is present but lacks an onChange handler to filter the organizations.
Apply this diff to implement the filtering:
+const [filteredResults, setFilteredResults] = useState(data?.results || []); <Input className="px-2 placeholder:text-xs placeholder:text-[#4B5563]" placeholder={t("filter_by_department_or_team_name")} + onChange={(e) => { + const searchTerm = e.target.value.toLowerCase(); + setFilteredResults( + data.results.filter(org => + org.name.toLowerCase().includes(searchTerm) + ) + ); + }} />Then update the data source in the table:
- {data.results + {filteredResults
🧹 Nitpick comments (2)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx (2)
175-218: Remove unnecessary Fragment wrapper.The Fragment (
<>...</>) is redundant here as it contains conditional rendering.Apply this diff to simplify the code:
- {isTopLevel && ( - <> - {children.length > 0 ? ( + {isTopLevel && children.length > 0 ? (🧰 Tools
🪛 Biome (1.9.4)
[error] 175-218: 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)
101-238: Consider performance optimizations for the OrganizationRow component.The component could benefit from performance optimizations:
- Memoize the component to prevent unnecessary re-renders
- Extract
toggleAllChildrento the parent componentApply this diff to optimize the component:
-const OrganizationRow = ({ +const OrganizationRow = React.memo(({ org, expandedRows, toggleRow, getChildren, indent, }) => { // ... component implementation -}; +});🧰 Tools
🪛 Biome (1.9.4)
[error] 175-218: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx
[error] 175-218: 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)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx (1)
42-42: Consider implementing proper pagination.Setting a high LIMIT (1000) without proper pagination could lead to performance issues with large datasets. Consider implementing server-side pagination with a reasonable page size.
Let's check if there are any existing pagination implementations in the codebase:
|
@nihal467 Review required |
| toggleChildren(child.id, expand); | ||
| }); | ||
| }; | ||
| const shouldExpand = !children.some( |
There was a problem hiding this comment.
| const shouldExpand = !children.some( | |
| const shouldExpand = !children.every( |
If user has expanded some and then clicks expand all, it should still expand rather than collapsing.
| icon="l-plus" | ||
| className="h-4 w-4 sm:h-2 sm:w-2" | ||
| /> | ||
| {t("expand_all")} |
There was a problem hiding this comment.
display collapse all if everything is expanded
| </TableRow> | ||
| </TableHeader> | ||
| <TableBody> | ||
| {data.results |
There was a problem hiding this comment.
For readability - do the filtering earlier and set to a diff variable and use that here.
|
|
||
| import Page from "@/components/Common/Page"; | ||
| import { CardGridSkeleton } from "@/components/Common/SkeletonLoading"; | ||
|
|
||
| import routes from "@/Utils/request/api"; | ||
| import query from "@/Utils/request/query"; | ||
| import { FacilityOrganization } from "@/types/facilityOrganization/facilityOrganization"; | ||
|
|
||
| import CreateFacilityOrganizationSheet from "./components/CreateFacilityOrganizationSheet"; |
public/locale/en.json
Outdated
| @@ -2027,6 +2032,8 @@ | |||
| "titrate_dosage": "Titrate Dosage", | |||
| "to": "to", | |||
| "to_be_conducted": "To be conducted", | |||
| "to_create_a_new_one": "to create a new one", | |||
| "to_open_manage_users_or_create_more_departments_teams_within_it": "to open manage users or create more departments/teams within it", | |||
There was a problem hiding this comment.
minor thing, but let's shorten this key 😄
| <> | ||
| <Button | ||
| variant="outline" | ||
| className="h-7 shadow-gray-400 border-gray-400 sm:p-2 sm:text-sm text-xs p-1" |
There was a problem hiding this comment.
Let's override styles, instead write a new variant if necessary.
In this case, white variant fits (not same as figma, but bit more subtle).
There was a problem hiding this comment.
try xs or sm size for expand/collapse button.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
…o issues/10374/Redesign-Root-View
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
public/locale/en.json (2)
569-569: Generic "click" Key Consideration
The key "click" (with the value "Click") is very generic. In a large-scale system or where similar actions occur in different contexts, consider using a more descriptive or namespaced key (e.g. "departments_click" if it’s specific to the Departments page) to avoid potential collisions and enhance clarity in translations.
1040-1040: Verbose Filter Key for Departments/Teams
The key "filter_by_department_or_team_name" and its value "Filter by department or team name" are descriptive, but they are on the verbose side. If this key is used frequently in the interface, consider shortening it (for example, "filter_department_team") to simplify usage while still conveying the essential meaning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
public/locale/en.json(6 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: OSSAR-Scan
- GitHub Check: CodeQL-Build
🔇 Additional comments (5)
public/locale/en.json (5)
293-293: Key Naming Consistency for Department/Team Addition
The new key "add_department_or_team" with the value "Add Department/Team" has been introduced. Note that there was previous feedback suggesting to shorten this key. Please confirm if this revised key meets the design conventions and naming consistency for the localization keys.
965-965: "Expand All" Localization Entry
The newly added key "expand_all" with the value "Expand All" clearly supports the functionality for expanding UI sections. The wording is concise and self-explanatory.
1899-1899: Clear "See Details" Localization Entry
The key "see_details" with the value "See Details" is clear and directly communicates its intended action to the user. This addition is straightforward and aligns with the redesigned UI’s needs.
2090-2090: "to_create_a_new_one" Text Verification
The fragment "to create a new one" appears to be used as helper text. Please verify that its grammatical structure and context are correct when it gets concatenated with surrounding text in the UI. Ensuring consistency in style (capitalization and punctuation, if needed) will help maintain clarity.
2091-2091: Lengthy Helper Text for Department/Team Management
The new key "to_open_manage_users_or_create_more_departments_teams_within_it" and its value "to open manage users or create more departments/teams within it" are quite descriptive. Please confirm that the phrasing fits well within the UI context (e.g., tooltips or guidance text) and consider simplifying it if possible. A shorter alternative might enhance readability and ease of translation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx (4)
95-100: Consider memoizing helper functions for performance.The
getChildrenfunction could benefit from memoization to avoid unnecessary recalculations, especially with large datasets.+const memoizedGetChildren = useMemo(() => + (parentId: string) => TableData.filter((org) => org.parent?.id === parentId), + [TableData] +);
176-211: Remove unnecessary Fragment wrapper.The Fragment is redundant as it contains only one child.
- <> {children.length > 0 ? ( <> <Button variant="white" size={"sm"} onClick={toggleAllChildren} > <CareIcon icon={allExpanded ? "l-minus" : "l-plus"} className="h-4 w-4 sm:h-2 sm:w-2" /> {t(allExpanded ? "collapse_all" : "expand_all")} </Button> <Button variant="white" size={"sm"} asChild> <Link href={`/departments/${org.id}`} className="text-[#030712] flex items-center" > <CareIcon icon="l-eye" className="h-4 w-4" /> {t("see_details")} </Link> </Button> </> ) : ( <Button variant="white" size={"sm"} asChild> <Link href={`/departments/${org.id}`} className="text-[#030712] flex items-center" > <CareIcon icon="l-eye" className="h-4 w-4" /> {t("see_details")} </Link> </Button> )} - </>🧰 Tools
🪛 Biome (1.9.4)
[error] 176-211: 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)
101-231: Consider using TypeScript interfaces for props.Define a proper interface for the OrganizationRow component props to improve type safety and documentation.
+interface Organization { + id: string; + name: string; + parent?: { id: string }; + org_type: string; +} +interface OrganizationRowProps { + org: Organization; + expandedRows: Record<string, boolean>; + toggleRow: (id: string) => void; + getChildren: (parentId: string) => Organization[]; + indent: number; +} -const OrganizationRow = ({ +const OrganizationRow: React.FC<OrganizationRowProps> = ({ org, expandedRows, toggleRow, getChildren, indent, -}: { - org: { - id: string; - name: string; - parent?: { id: string }; - org_type: string; - }; - expandedRows: Record<string, boolean>; - toggleRow: (id: string) => void; - getChildren: (parentId: string) => { - id: string; - name: string; - parent?: { id: string }; - org_type: string; - }[]; - indent: number; -})🧰 Tools
🪛 Biome (1.9.4)
[error] 176-211: 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)
268-294: Enhance table accessibility.Add ARIA labels and roles to improve accessibility for screen readers.
-<Table className="border rounded-lg w-full overflow-hidden"> +<Table + className="border rounded-lg w-full overflow-hidden" + aria-label="Organizations table" +> <TableHeader> <TableRow> - <TableHead className="w-[80%] border text-[#374151] bg-[#f3f4f6]"> + <TableHead + className="w-[80%] border text-[#374151] bg-[#f3f4f6]" + aria-sort="none" + > {t("name")} </TableHead> - <TableHead className="bg-[#f3f4f6] text-[#374151]"> + <TableHead + className="bg-[#f3f4f6] text-[#374151]" + aria-sort="none" + > {t("category")} </TableHead> </TableRow> </TableHeader>public/locale/en.json (1)
1041-1041: Consider shortening the translation key.The key
filter_by_department_or_team_nameis quite long. Consider using a shorter key likefilter_departmentswhile maintaining clarity.- "filter_by_department_or_team_name": "Filter by department or team name", + "filter_departments": "Filter by department or team name",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
public/locale/en.json(6 hunks)src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx
[error] 176-211: 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 (2)
src/pages/Facility/settings/organizations/FacilityOrganizationIndex.tsx (2)
251-267: LGTM! Well-structured informational section.The info section is well-implemented with clear instructions and proper visual hierarchy.
241-246:⚠️ Potential issueImplement the filter functionality for the search input.
The search input is present but lacks an onChange handler to filter the organizations.
<Input className="px-2 placeholder:text-xs placeholder:text-[#4B5563]" placeholder={t("filter_by_department_or_team_name")} + onChange={(e) => { + const searchTerm = e.target.value.toLowerCase(); + setFilteredResults( + TableData.filter(org => + org.name.toLowerCase().includes(searchTerm) + ) + ); + }} ></Input>Likely invalid or redundant comment.
|
Conflicts have been detected against the base branch. Please merge the base branch into your branch.
|
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit