FEATURE/HCMPRE-1806 : Attendance register #2115
Conversation
📝 WalkthroughWalkthroughThis pull request introduces several enhancements to the health campaign management module, focusing on attendance tracking and user management. The changes include updating the stylesheet version for health UI components, adding new routes for health-related services, and introducing new components such as 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 (
|
jagankumar-egov
changed the title
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/package.json
Outdated
Show resolved
Hide resolved
...b/micro-ui-internals/packages/modules/campaign-manager/src/components/IndividualUserTable.js
Outdated
Show resolved
Hide resolved
...ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Attendance.js
Show resolved
Hide resolved
...ro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/ProgressBar.js
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
♻️ Duplicate comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (1)
816-829: 🛠️ Refactor suggestionFix switch case declaration scope (duplicate).
The same scope issue exists in another switch statement.
Apply the same fix as above to this switch case.
🧰 Tools
🪛 Biome (1.9.4)
[error] 826-826: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
health/micro-ui/web/micro-ui-internals/packages/css/package.jsonis excluded by!**/*.json
📒 Files selected for processing (14)
health/micro-ui/web/micro-ui-internals/example/public/index.html(1 hunks)health/micro-ui/web/micro-ui-internals/example/src/setupProxy.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/IndividualUserTable.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/ProgressBar.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/tableCustomStyle.js(4 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js(4 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/index.js(2 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/services/searchAttendanceWithUserDetails.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useAttendanceWithUserDetails.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Attendance.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js(2 hunks)health/micro-ui/web/public/index.html(1 hunks)
🧰 Additional context used
📓 Path-based instructions (11)
health/micro-ui/web/micro-ui-internals/example/src/setupProxy.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/ProgressBar.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/index.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/tableCustomStyle.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/IndividualUserTable.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/services/searchAttendanceWithUserDetails.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/useAttendanceWithUserDetails.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Attendance.js (1)
Pattern **/*.js: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
[error] 477-477: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
[error] 826-826: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Attendance.js
[error] 138-138: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 139-139: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 140-140: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 141-141: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 311-311: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 312-316: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 320-327: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (11)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/services/searchAttendanceWithUserDetails.js (1)
6-14: Add error handling for API responsesAfter each API call, the code assumes a successful response with the expected data structure. To enhance reliability, add checks to verify the response status and data before processing.
For example, check if the response contains the expected properties:
if (!registerResponse || !registerResponse.attendanceRegister) { // Handle error or unexpected response }Also applies to: 63-75, 79-92
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/ProgressBar.js (1)
4-4: Consider adding theProgressBarcomponent todigit-ui-componentsAs previously suggested, if the
ProgressBarcomponent is needed across multiple modules or projects, consider adding it to the shareddigit-ui-componentslibrary for better reusability.health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/hooks/index.js (1)
22-22: LGTM! Clean integration of the attendance hook.The addition of
useAttendanceWithUserDetailsfollows the established pattern and is properly integrated into the campaign hooks collection.Also applies to: 52-52
health/micro-ui/web/micro-ui-internals/example/src/setupProxy.js (1)
100-102: LGTM! Verify the proxy endpoints.The new routes for health attendance and individual services are properly configured. Please ensure that these endpoints are accessible and properly configured in the target environment.
✅ Verification successful
Proxy endpoints verified successfully
The proxy endpoints are properly configured and actively used within the campaign-manager module for:
- Attendance register operations via
/health-attendance/v1/_search- Individual user management via
/health-individual/v1/_search🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the proxy endpoints are defined in the environment configuration # Check if the endpoints are defined in environment files rg -l "health-attendance|health-individual" .env* # Check for any related service configurations rg "health-attendance|health-individual" -g "*.json" -g "*.yaml" -g "*.yml"Length of output: 177
Script:
#!/bin/bash # Search for environment files and configurations in specific directories fd -e env -e env.js -e env.json . health/micro-ui/ # Search for these endpoints in JavaScript files rg -t js "health-attendance|health-individual" -A 2 -B 2 # Look for service configurations in package.json and other config files fd -e json -e yaml -e yml . health/micro-ui/ --exec grep -l "health-attendance\|health-individual" {} \;Length of output: 4044
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/tableCustomStyle.js (2)
Line range hint
72-171: Well-structured table styling with frozen column support!The new
getTableCustomStylefunction provides a robust solution for tables with frozen first columns. The implementation includes proper z-indexing and shadow effects for better visual separation.
3-12: 🧹 Nitpick (assertive)Consider keeping overflow properties for table containers.
Removing overflow properties might lead to layout issues with large datasets. Consider keeping them with appropriate values to handle content overflow gracefully.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js (1)
19-19: LGTM! Clean integration of the attendance route.The Attendance component is properly imported and integrated into the routing structure with appropriate access control using PrivateRoute.
Also applies to: 158-159
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/IndividualUserTable.js (1)
49-59: Review API configuration parameters.The API configuration has potential issues:
- Hard-coded limit of 1000 records could cause performance issues with large datasets
- No error handling for API failures
Run this script to check for other instances of hard-coded API limits:
✅ Verification successful
Hard-coded limit of 1000 records is consistent with the module's design
The API configuration's limit of 1000 records aligns with other similar endpoints in the campaign-manager module and appears to be an intentional design choice. Different modules in the codebase use varying limits (100-1000) based on their specific requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for hard-coded API limits in the codebase rg -A 2 'limit.*[0-9]{3,}'Length of output: 8966
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/Module.js (1)
181-183: LGTM! Component registration looks good.The new components are properly registered in the
componentsToRegisterobject.health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
15-15: LGTM: CSS version update for new attendance featuresThe version bump to 0.2.35 is consistent with the new styles added for attendance and progress bar components.
health/micro-ui/web/public/index.html (1)
13-13: LGTM: Consistent CSS version update across environmentsThe version bump to 0.2.35 maintains consistency with the example environment and includes the new attendance feature styles.
| const userDetails = staffResponse?.Individual.map(user => { | ||
| // Find the register that contains this staff member | ||
| const registerWithStaff = registerResponse.attendanceRegister.find( | ||
| register => register.staff?.[0]?.userId === user.id | ||
| ); | ||
|
|
||
| return { | ||
| id: user.id, | ||
| name: user?.name?.givenName, | ||
| dob: user.dateOfBirth, | ||
| email: user?.email, | ||
| mobileNumber: user?.mobileNumber, | ||
| registerId: registerWithStaff?.staff?.[0]?.registerId || null // Add registerId here | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Handle potential null values when mapping staffResponse.Individual
In the code:
const userDetails = staffResponse?.Individual.map(user => {
// ...
});If staffResponse?.Individual is null or undefined, using .map() will result in a TypeError. To prevent this, ensure that staffResponse?.Individual has a valid array value before mapping.
Apply this diff to handle potential null values:
- const userDetails = staffResponse?.Individual.map(user => {
+ const userDetails = (staffResponse?.Individual || []).map(user => {
// ...
});| individualResponse?.Individual.forEach(individual => { | ||
| attendeeDetailsMap[individual.id] = { | ||
| name: individual?.name?.givenName, | ||
| dob: individual.dateOfBirth, | ||
| email: individual?.email, | ||
| mobileNumber: individual?.mobileNumber, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Handle potential null values when iterating over individualResponse.Individual
In the code:
individualResponse?.Individual.forEach(individual => {
// ...
});If individualResponse?.Individual is null or undefined, using .forEach() will result in a TypeError. To prevent this, ensure that individualResponse?.Individual is a valid array before iterating.
Apply this diff to handle potential null values:
- individualResponse?.Individual.forEach(individual => {
+ (individualResponse?.Individual || []).forEach(individual => {
// ...
});| limit: 1000, | ||
| offset: 0, |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Avoid hardcoding limit values in API calls
The limit parameter is hardcoded to 1000 in both staff and attendee searches:
// Staff search
limit: 1000,
// Attendee search
limit: 1000,Hardcoding limits can lead to issues if the data exceeds this threshold. Consider making limit configurable or implementing pagination to handle larger datasets.
Also applies to: 83-84
| } | ||
|
|
||
| // Get staff user details | ||
| const staffResponse = await Digit.CustomService.getResponse({ | ||
| url: "/health-individual/v1/_search", | ||
| method: "POST", | ||
| params: { | ||
| limit: 1000, | ||
| offset: 0, | ||
| tenantId, | ||
| }, | ||
| body: { | ||
| Individual: { | ||
| id: Array.from(staffUserIds) | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
|
|
||
| // Get attendee details | ||
| const individualResponse = await Digit.CustomService.getResponse({ | ||
| url: "/health-individual/v1/_search", | ||
| method: "POST", | ||
| params: { | ||
| limit: 1000, | ||
| offset: 0, | ||
| tenantId, | ||
| }, | ||
| body: { | ||
| Individual: { | ||
| id: Array.from(individualIds) | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| // Create map of staff details | ||
| const userDetails = staffResponse?.Individual.map(user => { | ||
| // Find the register that contains this staff member | ||
| const registerWithStaff = registerResponse.attendanceRegister.find( | ||
| register => register.staff?.[0]?.userId === user.id | ||
| ); | ||
|
|
||
| return { | ||
| id: user.id, | ||
| name: user?.name?.givenName, | ||
| dob: user.dateOfBirth, | ||
| email: user?.email, | ||
| mobileNumber: user?.mobileNumber, | ||
| registerId: registerWithStaff?.staff?.[0]?.registerId || null // Add registerId here | ||
| }; | ||
| }); | ||
|
|
||
| // Create map of individual details | ||
| const attendeeDetailsMap = {}; | ||
| individualResponse?.Individual.forEach(individual => { | ||
| attendeeDetailsMap[individual.id] = { | ||
| name: individual?.name?.givenName, | ||
| dob: individual.dateOfBirth, | ||
| email: individual?.email, | ||
| mobileNumber: individual?.mobileNumber, | ||
| }; | ||
| }); | ||
|
|
||
| // Create a map for quick lookup of staff details | ||
| const staffDetailsMap = {}; | ||
| userDetails.forEach(user => { | ||
| staffDetailsMap[user.id] = user; | ||
| }); | ||
|
|
||
| // Enhance staffAttendeeMap with both staff and individual details | ||
| Object.keys(staffAttendeeMap).forEach(staffId => { | ||
| const enhancedStaffData = { | ||
| staffDetails: staffDetailsMap[staffId] || null, | ||
| attendees: staffAttendeeMap[staffId].map(attendee => ({ | ||
| ...attendee, | ||
| individualDetails: attendeeDetailsMap[attendee.individualId] || null | ||
| })) | ||
| }; | ||
| staffAttendeeMap[staffId] = enhancedStaffData; | ||
| }); | ||
|
|
||
| return { | ||
| registers: registerResponse.attendanceRegister, | ||
| staffAttendeeMap, | ||
| attendeeDetailsMap, | ||
| userDetails, | ||
| isError: false, | ||
| }; | ||
|
|
||
| } catch (error) { | ||
| console.error("Error in searchAttendanceWithUserDetails:", error); | ||
| return { | ||
| registers: [], | ||
| staffAttendeeMap: {}, | ||
| attendeeDetailsMap: {}, | ||
| userDetails: [], | ||
| isError: true, | ||
| error: error.message || "An unknown error occurred", | ||
| }; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Introduce constants for API endpoints and HTTP methods
Currently, the API URLs and methods are hardcoded within the function. For better maintainability and to avoid typos, consider defining constants for the API endpoints and HTTP methods.
Example:
const ATTENDANCE_SEARCH_URL = "/health-attendance/v1/_search";
const INDIVIDUAL_SEARCH_URL = "/health-individual/v1/_search";
const HTTP_POST = "POST";
// Usage in API calls
url: ATTENDANCE_SEARCH_URL,
method: HTTP_POST,| import React from 'react'; | ||
|
|
||
| const ProgressBar = ({ amount, total }) => { | ||
| const percentage = Math.min((amount / total) * 100, 100); |
There was a problem hiding this comment.
Handle potential division by zero in percentage calculation
In the calculation of percentage:
const percentage = Math.min((amount / total) * 100, 100);If total is 0, this will result in a division by zero, leading to Infinity or NaN. To prevent this, add a check to handle cases where total is zero.
Apply this diff to handle division by zero:
- const percentage = Math.min((amount / total) * 100, 100);
+ const percentage = total ? Math.min((amount / total) * 100, 100) : 0;| const numberA = parseInt(rowA.number, 10); | ||
| const numberB = parseInt(rowB.number, 10); | ||
| if (isNaN(numberA)) return 1; // Treat invalid numbers as larger | ||
| if (isNaN(numberB)) return -1; | ||
|
|
||
| if (numberA < numberB) return -1; | ||
| if (numberA > numberB) return 1; | ||
| return 0; | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix number parsing in sort function.
The sort function has potential issues:
- Uses unsafe
parseIntwithout radix - Uses unsafe
isNaNglobal function
Apply this diff to fix the issues:
- const numberA = parseInt(rowA.number, 10);
- const numberB = parseInt(rowB.number, 10);
- if (isNaN(numberA)) return 1;
- if (isNaN(numberB)) return -1;
+ const numberA = Number.parseInt(rowA.number, 10);
+ const numberB = Number.parseInt(rowB.number, 10);
+ if (Number.isNaN(numberA)) return 1;
+ if (Number.isNaN(numberB)) return -1;📝 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 numberA = parseInt(rowA.number, 10); | |
| const numberB = parseInt(rowB.number, 10); | |
| if (isNaN(numberA)) return 1; // Treat invalid numbers as larger | |
| if (isNaN(numberB)) return -1; | |
| if (numberA < numberB) return -1; | |
| if (numberA > numberB) return 1; | |
| return 0; | |
| }, | |
| const numberA = Number.parseInt(rowA.number, 10); | |
| const numberB = Number.parseInt(rowB.number, 10); | |
| if (Number.isNaN(numberA)) return 1; // Treat invalid numbers as larger | |
| if (Number.isNaN(numberB)) return -1; | |
| if (numberA < numberB) return -1; | |
| if (numberA > numberB) return 1; | |
| return 0; | |
| }, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 139-139: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 140-140: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 141-141: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
| case "ACTION_LABEL_CONFIGURE_REGISTER": | ||
| window.history.pushState( | ||
| { | ||
| name: row?.campaignName, | ||
| data: row, | ||
| projectId: row?.projectId, | ||
| }, | ||
| "", | ||
| `/${window.contextPath}/employee/campaign/attendance` | ||
| ); | ||
| const navEvent2 = new PopStateEvent("popstate"); | ||
| window.dispatchEvent(navEvent2); | ||
| break; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix switch case declaration scope.
The navEvent2 declaration could leak into other switch cases. Wrap the case in a block to restrict its scope.
Apply this diff to fix the scope issue:
case "ACTION_LABEL_CONFIGURE_REGISTER":
+ {
window.history.pushState(
{
name: row?.campaignName,
data: row,
projectId: row?.projectId,
},
"",
`/${window.contextPath}/employee/campaign/attendance`
);
const navEvent2 = new PopStateEvent("popstate");
window.dispatchEvent(navEvent2);
break;
+ }📝 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.
| case "ACTION_LABEL_CONFIGURE_REGISTER": | |
| window.history.pushState( | |
| { | |
| name: row?.campaignName, | |
| data: row, | |
| projectId: row?.projectId, | |
| }, | |
| "", | |
| `/${window.contextPath}/employee/campaign/attendance` | |
| ); | |
| const navEvent2 = new PopStateEvent("popstate"); | |
| window.dispatchEvent(navEvent2); | |
| break; | |
| case "ACTION_LABEL_CONFIGURE_REGISTER": | |
| { | |
| window.history.pushState( | |
| { | |
| name: row?.campaignName, | |
| data: row, | |
| projectId: row?.projectId, | |
| }, | |
| "", | |
| `/${window.contextPath}/employee/campaign/attendance` | |
| ); | |
| const navEvent2 = new PopStateEvent("popstate"); | |
| window.dispatchEvent(navEvent2); | |
| break; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 477-477: Other switch clauses can erroneously access this declaration.
Wrap the declaration in a block to restrict its access to the switch clause.
The declaration is defined in this switch clause:
Unsafe fix: Wrap the declaration in a block.
(lint/correctness/noSwitchDeclarations)
| .progress-bar-fill { | ||
| height: 100%; | ||
| background-color: #2BD27F; /* Use your PRIMARY_COLOR */ |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Use theme variables for color consistency
Replace the hardcoded color with a theme variable to maintain consistency with the design system.
- background-color: #2BD27F; /* Use your PRIMARY_COLOR */
+ background-color: theme(digitv2.lightTheme.success);Committable suggestion skipped: line range outside the PR's diff.
| .attendanceCell { | ||
| display: flex; | ||
| } | ||
|
|
||
| .chip-container { | ||
| display: flex; | ||
| align-items: center; | ||
| gap: 0.5rem; /* Adjust the spacing between chips and the button */ | ||
| } | ||
|
|
||
| .chip-wrapper { | ||
| display: inline-flex; | ||
| } | ||
|
|
||
| .more-button { | ||
| margin-left: 0.5rem; /* Adjust the spacing between chips and the + button */ | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider improving chip container accessibility and responsiveness
The attendance cell and chip container implementation could benefit from some improvements:
- Add ARIA attributes for better accessibility
- Consider responsive design for smaller screens
- Add hover states for interactive elements
.attendanceCell {
display: flex;
+ @media (max-width: 768px) {
+ flex-direction: column;
+ }
}
.chip-container {
display: flex;
align-items: center;
gap: 0.5rem;
+ flex-wrap: wrap;
}
.chip-wrapper {
display: inline-flex;
+ &:hover {
+ opacity: 0.9;
+ }
}
.more-button {
margin-left: 0.5rem;
+ cursor: pointer;
+ &:hover {
+ opacity: 0.8;
+ }
}📝 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.
| .attendanceCell { | |
| display: flex; | |
| } | |
| .chip-container { | |
| display: flex; | |
| align-items: center; | |
| gap: 0.5rem; /* Adjust the spacing between chips and the button */ | |
| } | |
| .chip-wrapper { | |
| display: inline-flex; | |
| } | |
| .more-button { | |
| margin-left: 0.5rem; /* Adjust the spacing between chips and the + button */ | |
| } | |
| .attendanceCell { | |
| display: flex; | |
| @media (max-width: 768px) { | |
| flex-direction: column; | |
| } | |
| } | |
| .chip-container { | |
| display: flex; | |
| align-items: center; | |
| gap: 0.5rem; /* Adjust the spacing between chips and the button */ | |
| flex-wrap: wrap; | |
| } | |
| .chip-wrapper { | |
| display: inline-flex; | |
| &:hover { | |
| opacity: 0.9; | |
| } | |
| } | |
| .more-button { | |
| margin-left: 0.5rem; /* Adjust the spacing between chips and the + button */ | |
| cursor: pointer; | |
| &:hover { | |
| opacity: 0.8; | |
| } | |
| } |
| /* Container for the progress bar */ | ||
| .progress-bar-container { | ||
| position: relative; | ||
| width: 20rem; | ||
| max-width: 20rem; | ||
| height: 20px; | ||
| background-color: #f3f3f3; | ||
| border-radius: 10px; | ||
| overflow: hidden; | ||
| border: 1px solid #d6d5d4; | ||
| } | ||
|
|
||
| /* The fill of the progress bar */ | ||
| .progress-bar-fill { | ||
| height: 100%; | ||
| background-color: #2BD27F; /* Use your PRIMARY_COLOR */ | ||
| transition: width 0.4s ease-in-out; | ||
| display: flex; | ||
| align-items: center; | ||
| justify-content: center; | ||
| } | ||
|
|
||
| /* Text inside the progress bar */ | ||
| .progress-bar-text { | ||
| position: absolute; | ||
| width: 100%; | ||
| text-align: center; | ||
| font-size: 14px; | ||
| color: #fff; | ||
| font-weight: bold; | ||
| pointer-events: none; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance progress bar accessibility and animation
The progress bar implementation needs accessibility improvements and could benefit from smoother animations:
- Add ARIA attributes for screen readers
- Consider adding a tooltip for precise values
- Improve color contrast for better visibility
.progress-bar-container {
position: relative;
width: 20rem;
max-width: 20rem;
height: 20px;
background-color: #f3f3f3;
border-radius: 10px;
overflow: hidden;
border: 1px solid #d6d5d4;
+ role: "progressbar";
+ aria-valuemin: "0";
+ aria-valuemax: "100";
}
.progress-bar-fill {
height: 100%;
background-color: #2BD27F;
- transition: width 0.4s ease-in-out;
+ transition: width 0.3s cubic-bezier(0.4, 0, 0.2, 1);
display: flex;
align-items: center;
justify-content: center;
}
.progress-bar-text {
position: absolute;
width: 100%;
text-align: center;
font-size: 14px;
- color: #fff;
+ color: #000;
+ text-shadow: 0 0 2px #fff;
font-weight: bold;
pointer-events: none;
+ user-select: none;
}Committable suggestion skipped: line range outside the PR's diff.
...ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Attendance.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/IndividualUserTable.js (2)
101-107:⚠️ Potential issueImplement the
handleAssignandhandleUnassignfunctions.The
handleAssignandhandleUnassignfunctions are currently placeholders with comments indicating that the code needs to be written. These functions are critical for the component's functionality to assign and unassign users.
246-248: 🛠️ Refactor suggestionFix hardcoded progress bar values.
The progress bar values (
amount={40}andtotal={100}) are hardcoded. These should be dynamically calculated based on the actual data to accurately reflect the assignment progress.Apply this diff to fix the issue:
- <ProgressBar amount={40} total={100} /> - <span style={{ fontWeight: 600 }}>{`${40}%`}</span> + <ProgressBar amount={staffAttendeeIds.length} total={data?.Individual?.length || 0} /> + <span style={{ fontWeight: 600 }}>{`${data?.Individual?.length ? Math.round((staffAttendeeIds.length / data.Individual.length) * 100) : 0}%`}</span>health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Attendance.js (2)
138-146: 🛠️ Refactor suggestionUse
Number.parseIntandNumber.isNaNfor number parsing and checking.Using
Number.parseIntandNumber.isNaNis safer and more consistent than the globalparseIntandisNaNfunctions. This avoids unexpected type coercion and improves code reliability.Apply this diff to fix the issues:
- const numberA = parseInt(rowA.number, 10); - const numberB = parseInt(rowB.number, 10); - if (isNaN(numberA)) return 1; - if (isNaN(numberB)) return -1; + const numberA = Number.parseInt(rowA.number, 10); + const numberB = Number.parseInt(rowB.number, 10); + if (Number.isNaN(numberA)) return 1; + if (Number.isNaN(numberB)) return -1;🧰 Tools
🪛 Biome (1.9.4)
[error] 138-138: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
[error] 139-139: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.(lint/style/useNumberNamespace)
[error] 140-140: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
[error] 141-141: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
321-328:⚠️ Potential issueAdd
keyprop to elements in array.When rendering elements in an array, React requires a unique
keyprop for each element to optimize rendering and prevent potential issues.Apply this diff to fix the issue:
footerChildren={[ + <Button + key="close-button" type="button" size="large" variation="secondary" label={t("REGISTER_CLOSE")} onClick={() => setAssignUsersPopup(false)} style={{ minWidth: "200px" }} /> ]}🧰 Tools
🪛 Biome (1.9.4)
[error] 321-328: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss (2)
236-252: 🛠️ Refactor suggestionEnhance accessibility and responsiveness of attendance components
The attendance cell and chip container implementation needs improvements for better accessibility and responsiveness.
.attendanceCell { display: flex; + @media (max-width: 768px) { + flex-direction: column; + } } .chip-container { display: flex; align-items: center; gap: 0.5rem; + flex-wrap: wrap; + role: "group"; + aria-label: "Attendance chips"; } .chip-wrapper { display: inline-flex; + &:hover { + opacity: 0.9; + } } .more-button { margin-left: 0.5rem; + cursor: pointer; + &:hover { + opacity: 0.8; + } + &:focus { + outline: 2px solid theme(digitv2.lightTheme.primary); + } }
254-285: 🛠️ Refactor suggestionImprove progress bar accessibility and theming
The progress bar implementation needs accessibility improvements and should use theme variables.
.progress-bar-container { position: relative; width: 20rem; max-width: 20rem; height: 1.5rem; background-color: #f3f3f3; border-radius: 1rem; overflow: hidden; border: 1px solid #d6d5d4; + role: "progressbar"; + aria-valuemin: "0"; + aria-valuemax: "100"; } .progress-bar-fill { height: 100%; - background-color: #2BD27F; + background-color: theme(digitv2.lightTheme.success); transition: width 0.4s ease-in-out; display: flex; align-items: center; justify-content: center; } .progress-bar-text { position: absolute; width: 100%; text-align: center; font-size: 1rem; - color: #fff; + color: theme(digitv2.lightTheme.text); font-weight: bold; pointer-events: none; + user-select: none; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/IndividualUserTable.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/tableCustomStyle.js(5 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Attendance.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js(3 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Attendance.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/tableCustomStyle.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/IndividualUserTable.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js (1)
Pattern **/*.js: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Attendance.js
[error] 138-138: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 139-139: Use Number.parseInt instead of the equivalent global.
ES2015 moved some globals into the Number namespace for consistency.
Safe fix: Use Number.parseInt instead.
(lint/style/useNumberNamespace)
[error] 140-140: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 141-141: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
[error] 312-312: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 313-317: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
[error] 321-328: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.
(lint/correctness/useJsxKeyInIterable)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js
[error] 86-86: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/tableCustomStyle.js (1)
Line range hint
3-175: Styling updates look good.The updates to the custom table styles improve consistency and responsiveness by using
remunits and adjusting borders. The refactoring enhances readability and maintainability of the styling code.health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss (1)
287-291: LGTM! Font family specification for popup headingThe font family specification for the popup heading is correctly implemented.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/index.js (2)
19-19: LGTM! Import statement for Attendance componentThe import statement is correctly placed and follows the existing import pattern.
163-164: LGTM! Route configuration for Attendance componentThe route configuration follows the existing pattern and is correctly implemented.
| heading={`${t("REGISTER_ASSIGN_USERS_TO_SUPERVISOR")} ${curSupervisor}`} | ||
| headerclassName={"assign-users-popup"} | ||
| children={[ |
There was a problem hiding this comment.
Avoid passing children as a prop to PopUp component.
Passing children as a prop is not standard practice in React. Instead, use JSX to include child components within the opening and closing tags of the PopUp component.
Apply this diff to fix the issue:
<PopUp
className="roleComposer"
type="default"
- heading={`${t("REGISTER_ASSIGN_USERS_TO_SUPERVISOR")} ${curSupervisor}`}
- headerclassName={"assign-users-popup"}
- children={[
+ heading={`${t("REGISTER_ASSIGN_USERS_TO_SUPERVISOR")} ${curSupervisor}`}
+ headerclassName={"assign-users-popup"}
+ >
+ <IndividualUserTable
tenantId={tenantId}
staffAttendeeIds={curAttendeesList}
supervisorName={curSupervisor}
/>
- ]}
onOverlayClick={() => setAssignUsersPopup(false)}Ensure that you close the PopUp component appropriately:
</PopUp>🧰 Tools
🪛 Biome (1.9.4)
[error] 312-312: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
| <span style={{ fontWeight: 600 }}>{t("REGISTER_ASSIGNED_USERS")}</span> | ||
| <ProgressBar amount={40} total={100} /> | ||
| <span style={{ fontWeight: 600 }}>{`${40}%`}</span> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix hardcoded progress bar values.
The progress bar values (amount={40} and total={100}) are hardcoded. These should be dynamically calculated based on actual data to accurately represent the users' assignment progress.
Apply this diff to fix the issue:
- <ProgressBar amount={40} total={100} />
- <span style={{ fontWeight: 600 }}>{`${40}%`}</span>
+ <ProgressBar amount={assignedUsersCount} total={totalUsersCount} />
+ <span style={{ fontWeight: 600 }}>{`${totalUsersCount ? Math.round((assignedUsersCount / totalUsersCount) * 100) : 0}%`}</span>Note: You need to calculate assignedUsersCount and totalUsersCount based on your data.
Committable suggestion skipped: line range outside the PR's diff.
| }, | ||
| { | ||
| path: "", | ||
| content: t("ACTION_LABEL_CONFIGURE_REGISTER"), | ||
| show: pathVar === "attendance" ? true : false |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Simplify boolean expression in breadcrumb configuration
The boolean expression can be simplified for better readability.
{
path: "",
content: t("ACTION_LABEL_CONFIGURE_REGISTER"),
- show: pathVar === "attendance" ? true : false
+ show: pathVar === "attendance"
}📝 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.
| }, | |
| { | |
| path: "", | |
| content: t("ACTION_LABEL_CONFIGURE_REGISTER"), | |
| show: pathVar === "attendance" ? true : false | |
| }, | |
| { | |
| path: "", | |
| content: t("ACTION_LABEL_CONFIGURE_REGISTER"), | |
| show: pathVar === "attendance" |
🧰 Tools
🪛 Biome (1.9.4)
[error] 86-86: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
Choose the appropriate template for your PR:
Feature PR
Feature Request
JIRA ID
Module
Description
Related Issues
Bugfix PR
Bugfix Request
JIRA ID
Module
Description
Root Cause
Related Issues
Release PR
Summary by CodeRabbit
New Features
Style Updates
Routing
Performance