planInbox console #1614
planInbox console #1614
Conversation
📝 WalkthroughWalkthroughThis pull request introduces significant updates to the styling and functionality of several components within the micro UI framework. Key changes include the addition of new CSS classes and styles in Changes
Possibly related PRs
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/RoleTableComposer.js (2)
Line range hint
137-176: LGTM: ImprovedhandleHierarchyChangeimplementationThe refactored
handleHierarchyChangefunction now handles both existing and new rows more robustly. It correctly uses the functional update form ofsetRowDataand maintains other properties of existing rows when updating.Consider extracting the logic for creating a new row into a separate function to improve readability:
const createNewRow = (rowIndex, value, boundaryOptions) => ({ rowIndex, selectedHierarchy: value, boundaryOptions, selectedBoundaries: [], }); // Then in the else block: return [...prev, createNewRow(row?.rowIndex, value, boundaryOptions)];
Line range hint
177-241: LGTM: EnhancedhandleBoundaryChangeimplementationThe refactored
handleBoundaryChangefunction now robustly handles both existing and new rows. It correctly uses the functional update form ofsetRowDataand maintains other properties of existing rows when updating.For consistency with the
handleHierarchyChangefunction, consider extracting the logic for creating a new row into a separate function:const createNewBoundaryRow = (rowIndex, boundariesInEvent) => ({ rowIndex, selectedBoundaries: boundariesInEvent, }); // Then in the else block: return [...prev, createNewBoundaryRow(row?.rowIndex, boundariesInEvent)];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (6)
- health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/RoleTableComposer.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UserAccess.js (5 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/index.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/searchPlanWithCensus.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/usePlanSearchWithCensus.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/RoleTableComposer.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UserAccess.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/index.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/services/searchPlanWithCensus.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/usePlanSearchWithCensus.js (1)
Pattern
**/*.js: check
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (3)
Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#876 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:1940-2392 Timestamp: 2024-06-14T14:10:38.086Z Learning: Classes related to interactive elements in the microplan preview section are mostly passed to Higher Order Components (HOCs), and ARIA attributes for non-HOC elements will be managed directly by adding them where necessary.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#876 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:1940-2392 Timestamp: 2024-10-08T20:11:07.772Z Learning: Classes related to interactive elements in the microplan preview section are mostly passed to Higher Order Components (HOCs), and ARIA attributes for non-HOC elements will be managed directly by adding them where necessary.Learnt from: siddhant-nawale-egov PR: egovernments/DIGIT-Frontend#876 File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:1940-2392 Timestamp: 2024-10-08T20:11:12.539Z Learning: Classes related to interactive elements in the microplan preview section are mostly passed to Higher Order Components (HOCs), and ARIA attributes for non-HOC elements will be managed directly by adding them where necessary.
🔇 Additional comments (11)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/usePlanSearchWithCensus.js (2)
1-2: LGTM: Imports are appropriate and well-structured.The imports are correct and necessary for the hook's functionality. Using
react-queryis a good practice for managing server state, and importing a local service function promotes separation of concerns.
13-13: LGTM: Appropriate export statement.The default export is correct for a single hook in the file, allowing for easy import and potential renaming in other parts of the application.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/hooks/index.js (2)
20-20: LGTM: New hook import looks good.The import statement for
usePlanSearchWithCensusfollows the established pattern and naming convention in this file.
40-40: LGTM: New hook correctly added to microplanv1 object.The
usePlanSearchWithCensushook has been properly added to themicroplanv1object, maintaining consistency with the existing structure.health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UserAccess.js (4)
7-7: LGTM: Improved function signature formatting.The removal of extra spaces between parameters enhances code readability and consistency.
98-98: Please clarify the removal of the "ROOT" category check.The selector for the "ADMINISTRATIVE_HIERARCHY" column has been simplified by removing the condition that checks if the category starts with "ROOT".
Can you confirm if this change is intentional and if the "COUNTRY" hierarchy level is no longer needed? If it's still required, consider keeping the condition or explain why it's been removed.
148-148: LGTM: Added onChangeRowsPerPage prop to DataTable.The addition of the
onChangeRowsPerPageprop to the DataTable component correctly connects it to thehandleRowsPerPageChangefunction, enabling user control over the number of rows displayed per page.
Line range hint
1-188: Overall assessment: Improved pagination and code quality.The changes in this file significantly enhance the
UserAccesscomponent's pagination functionality and overall code quality. The introduction of therowsPerPagestate and associated handler function provides more flexible control over data display. The code formatting improvements contribute to better readability and maintainability.However, there are a few minor suggestions for further improvement:
- Consider using a constant for the initial
rowsPerPagevalue.- Add a comment explaining the page reset in
handleRowsPerPageChange.- Clarify the removal of the "ROOT" category check in the "ADMINISTRATIVE_HIERARCHY" column selector.
These changes align well with the PR objectives of fixing issues related to page size changes and overall improvement of the planInbox console.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/RoleTableComposer.js (3)
20-20: LGTM: State management improvement for paginationThe addition of the
setRowsPerPagesetter function enables dynamic updates to the number of rows per page, which is crucial for the new pagination functionality.
434-434: LGTM: DataTable pagination enhancementThe addition of the
onChangeRowsPerPageprop to the DataTable component correctly integrates the newhandleRowsPerPageChangefunction, enabling dynamic updates to the number of rows per page.
Line range hint
1-447: Overall assessment: Improved pagination and state managementThe changes in this file significantly enhance the
RoleTableComposercomponent's functionality, particularly in terms of pagination and state management. The refactoredhandleHierarchyChangeandhandleBoundaryChangefunctions now handle both existing and new rows more robustly, while the newhandleRowsPerPageChangefunction and associated DataTable prop enable dynamic updates to the number of rows per page.These improvements contribute to a more responsive and user-friendly interface, aligning well with the PR objectives of enhancing the planInbox console functionality.
| @@ -0,0 +1,13 @@ | |||
| import { useQuery } from "react-query"; | |||
| import searchPlanWithCensus from "./services/searchPlanWithCensus"; | |||
| const usePlanSearchWithCensus = ({ tenantId, microplanId, body, limit = 5, offset = 0, roles, config = {} }) => { | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Well-structured hook definition with a minor suggestion.
The hook definition follows React conventions and uses object destructuring for better readability. The default values for limit and offset are sensible.
Consider adding input validation for limit and offset to ensure they are non-negative integers. This could prevent potential issues with invalid inputs.
| return useQuery( | ||
| ["PLAN_SEARCH_EMPLOYEE_WITH_TAGGING", tenantId, microplanId, body, limit, offset, roles, config?.queryKey], | ||
| () => searchPlanWithCensus({ tenantId, microplanId, body, limit, offset, roles }), | ||
| { | ||
| ...config, | ||
| } | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Solid implementation of useQuery with a suggestion for improvement.
The useQuery implementation is well-structured, with a comprehensive query key for proper caching and correct parameter passing. The use of config spreading allows for flexible configuration.
Consider adding error handling within the query function or in the component using this hook. This could improve the user experience by providing meaningful error messages or fallback UI in case of API failures.
Example:
() => searchPlanWithCensus({ tenantId, microplanId, body, limit, offset, roles })
.catch(error => {
console.error('Error fetching plan with census:', error);
throw error; // Re-throw to let react-query handle it
})| const { t } = useTranslation(); | ||
| const tenantId = Digit.ULBService.getCurrentTenantId(); | ||
| const { campaignId, microplanId, key, ...queryParams } = Digit.Hooks.useQueryParams(); | ||
| const [showPopUp, setShowPopUp] = useState(null); | ||
| const [showToast, setShowToast] = useState(null); | ||
| const [currentPage, setCurrentPage] = useState(1); | ||
| const [rowsPerPage] = useState(5); | ||
| const [rowsPerPage, setRowsPerPage] = useState(5); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Added state for rows per page.
The introduction of the rowsPerPage state variable enhances the component's flexibility by allowing dynamic control of the number of rows displayed per page.
Consider using a constant or configuration value for the initial rowsPerPage value (5) to improve maintainability.
| const handleRowsPerPageChange = (newPerPage, page) => { | ||
| setRowsPerPage(newPerPage); // Update the rows per page state | ||
| setCurrentPage(page); // Optionally reset the current page or maintain it | ||
| refetchPlanEmployee(); | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: Added handler for rows per page change.
The handleRowsPerPageChange function properly updates the rowsPerPage state and triggers a data refetch, improving the component's responsiveness to user interactions.
Consider adding a comment explaining the purpose of resetting the current page when the rows per page changes, as it might not be immediately obvious to other developers.
| const handleRowsPerPageChange = (newPerPage, page) => { | ||
| setRowsPerPage(newPerPage); // Update the rows per page state | ||
| setCurrentPage(page); // Optionally reset the current page or maintain it | ||
| refetchHrms(); // Fetch the updated data with the new rows per page | ||
| }; |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM: New handleRowsPerPageChange function added
The new handleRowsPerPageChange function properly updates the rowsPerPage state and triggers a data refetch, which is crucial for the pagination functionality.
Consider making the page reset optional:
const handleRowsPerPageChange = (newPerPage, page) => {
setRowsPerPage(newPerPage);
if (page !== currentPage) {
setCurrentPage(page);
}
refetchHrms();
};This way, the current page is only updated if it has actually changed, potentially reducing unnecessary re-renders.
| if (error?.response?.data?.Errors) { | ||
| throw new Error(error.response.data.Errors[0].message); | ||
| } | ||
| throw new Error("An unknown error occurred"); |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Include original error message in unknown errors
When throwing the generic error, include the original error message to provide more context for debugging.
Apply this diff:
- throw new Error("An unknown error occurred");
+ throw new Error(error.message || "An unknown error occurred");📝 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.
| throw new Error("An unknown error occurred"); | |
| throw new Error(error.message || "An unknown error occurred"); |
| if (!response) { | ||
| throw new Error("Employee not found with the given role"); | ||
| } |
There was a problem hiding this comment.
Correct the error message when plan data is not found
The error message "Employee not found with the given role" is misleading. Since this function searches for plan data, update the error message to accurately reflect the issue.
Apply this diff:
-if (!response) {
- throw new Error("Employee not found with the given role");
+if (!response || !response.Plan || response.Plan.length === 0) {
+ throw new Error("Plan data not found");📝 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.
| if (!response) { | |
| throw new Error("Employee not found with the given role"); | |
| } | |
| if (!response || !response.Plan || response.Plan.length === 0) { | |
| throw new Error("Plan data not found"); | |
| } |
| throw new Error("Employee not found with the given role"); | ||
| } | ||
|
|
||
| const localityArray = [...new Set(response?.Plan?.map((item) => item?.locality))]; |
There was a problem hiding this comment.
Handle undefined locality values when mapping response.Plan
If item.locality is undefined, it will be included in localityArray as undefined. Filter out undefined localities to avoid potential issues in the subsequent API call.
Apply this diff:
-const localityArray = [...new Set(response?.Plan?.map((item) => item?.locality))];
+const localityArray = [...new Set(response?.Plan?.map((item) => item?.locality).filter(Boolean))];📝 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 localityArray = [...new Set(response?.Plan?.map((item) => item?.locality))]; | |
| const localityArray = [...new Set(response?.Plan?.map((item) => item?.locality).filter(Boolean))]; |
| if (!fetchCensusData) { | ||
| throw new Error("Employee not found with the given role"); | ||
| } |
There was a problem hiding this comment.
Correct the error message when census data is not found
The error message "Employee not found with the given role" is misleading. Since this function fetches census data, update the error message to accurately reflect the issue.
Apply this diff:
-if (!fetchCensusData) {
- throw new Error("Employee not found with the given role");
+if (!fetchCensusData || !fetchCensusData.Census || fetchCensusData.Census.length === 0) {
+ throw new Error("Census data not found");📝 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.
| if (!fetchCensusData) { | |
| throw new Error("Employee not found with the given role"); | |
| } | |
| if (!fetchCensusData || !fetchCensusData.Census || fetchCensusData.Census.length === 0) { | |
| throw new Error("Census data not found"); | |
| } |
| .digit-multiselectdropdown-wrap.nestedmultiselect{ | ||
| min-width: 30rem; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using responsive units for min-width
The hard-coded min-width: 30rem; may not be optimal for all screen sizes. To enhance responsiveness, consider using relative units like percentages (%) or em, or apply media queries to adjust the min-width based on screen dimensions.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation