-
Notifications
You must be signed in to change notification settings - Fork 19
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Console v0.3 changes #1823
base: master
Are you sure you want to change the base?
Console v0.3 changes #1823
Conversation
* removed iterator * pull from console * Microplan search working * added unstaged files * Error field removed forn config
* added plan with census call hook * added page size change fix * remove commit * added plan inbox screen * planInbox resolve * ws --------- Co-authored-by: NabeelAyubee <[email protected]>
Co-authored-by: NabeelAyubee <[email protected]>
* clearing session appropriately and updated workflow * removing unnecessary keys from url --------- Co-authored-by: nabeelmd-eGov <[email protected]>
* cleanup and roles from mdms v2 * Update CreateQuestionContext.js * using global config for mdms-v2 and update checklist code modified * Update health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewChecklist.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/SearchChecklist.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/ViewChecklist.js Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> --------- Co-authored-by: Jagankumar <[email protected]> Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* added showtab enhance * issue fix * css update * update styles * added chip pop --------- Co-authored-by: NabeelAyubee <[email protected]>
…ow next actions (#1640) * Choose Activity - Screen Validations based on Business service wokrflow next states * Added null checks
* Finalise Action for Facility Catchment + Validations * Update hardcoded tenant id * Updated check for total count --------- Co-authored-by: rachana-egov <[email protected]>
* added showtab enhance * issue fix * css update * update styles * added chip pop * added pagination --------- Co-authored-by: NabeelAyubee <[email protected]>
* removed iterator * Changes in card layout and datamgmgt screen * random commit * making datamgmt, registering components
* hierarchy * loader added * cleanup
Co-authored-by: NabeelAyubee <[email protected]>
Finalise Action for Estimation Inbox+ Validations Co-authored-by: rachana-egov <[email protected]>
* Draft initilised * Draft changes * Draft Changes * Changed options * Changed ui customizations
#1656) * fixed workflow buttons for pop and plan screen and updated back button for village screen * removed extra refetch calls --------- Co-authored-by: rachana-egov <[email protected]>
* added condition based rendering of edit column * added props.showEditColumn as use memo dependency --------- Co-authored-by: rachana-egov <[email protected]>
#1659) invalidating files on boundary change, invalidation assumptions and formulas on bou Co-authored-by: Nipun Arora <[email protected]>
* fixed facility pagination issue * fixed toast issue * fixed table pagination issue * disabled submit button until some changes are made inside security and accesibility popup * resolved coderabbit comments --------- Co-authored-by: rachana-egov <[email protected]>
* draft issue fix, table reload fix, table no data fix * key change to planConfiguration ID * change to plans --------- Co-authored-by: NabeelAyubee <[email protected]>
* DEMO UI Bug fixes * Removed console logs * Sort default by lastModifiedTime for hrms search * Added sort params in example UI Customizations --------- Co-authored-by: Nipun Arora <[email protected]>
* updated pending for validation for default in filter card * removed console logs --------- Co-authored-by: rachana-egov <[email protected]>
…1667) * updated table styles and added customhook for plan facility search * commented dummy data
* draft issue fix, table reload fix, table no data fix * key change to planConfiguration ID * change to plans * added new enhcance in plan inbox --------- Co-authored-by: NabeelAyubee <[email protected]>
…#1873) Disabled updating security and accessibility details after finalize facility catchment Co-authored-by: rachana-egov <[email protected]>
Co-authored-by: Nipun Arora <[email protected]>
Co-authored-by: Nipun Arora <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
health/micro-ui/web/micro-ui-internals/packages/css/package.json
is excluded by!**/*.json
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/example/public/index.html
(2 hunks)
🔇 Additional comments (1)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
13-15
: Verify version compatibility and test for breaking changes
The stylesheet versions have been significantly updated:
- digit-ui-css: Various beta versions → 1.8.3
- digit-ui-components-css: 0.0.2-beta.13 → 0.0.2-beta.41
- Added new health-specific CSS: 0.1.40
These major version jumps could introduce breaking changes.
✅ Verification successful
Let me try another approach to verify the version compatibility by checking the git history and package dependencies.
CSS package updates appear to be compatible with the codebase
Based on the analysis of package.json files and imports:
- The health-specific CSS package
@egovernments/[email protected]
is properly referenced in the codebase - The component dependencies in various modules are aligned with the CSS versions:
- digit-ui-react-components: 1.8.2-beta.18 to 1.8.10
- digit-ui-components: 0.0.2-beta.49 to 0.0.2-beta.54
- No custom CSS overrides were found that could be affected by the version updates
The version updates appear to be part of a coordinated upgrade across the health module ecosystem.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential breaking changes in the updated packages
# Search for any custom CSS overrides that might be affected
echo "Searching for custom CSS overrides..."
rg -g "*.scss" -g "*.css" -g "!dist/*" -g "!node_modules/*" \
-l "@egovernments/digit-ui-css|@egovernments/digit-ui-components-css"
# Search for version constraints in package.json files
echo "Checking package version constraints..."
fd "package.json" -x jq -r '.dependencies |
select(.["@egovernments/digit-ui-css"] or
.["@egovernments/digit-ui-components-css"] or
.["@egovernments/digit-ui-health-css"])'
Length of output: 500
Script:
#!/bin/bash
# Check git history for the specific file to understand version changes
git log -p -- health/micro-ui/web/micro-ui-internals/example/public/index.html | grep -A 3 "digit-ui-.*css"
# Look for package.json files in the health module
fd package.json health/micro-ui/web/micro-ui-internals/ -x cat {}
# Search for any CSS import statements or style dependencies
rg -g "*.js" -g "*.jsx" -g "!dist/*" -g "!node_modules/*" \
"@egovernments/digit-ui-(css|components-css|health-css)" health/micro-ui/web/micro-ui-internals/
Length of output: 10678
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | ||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | ||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | ||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Document the CSS module dependency hierarchy
The addition of digit-ui-health-css
suggests a feature-specific CSS module. Consider documenting:
- The relationship between base CSS and feature-specific CSS
- Load order dependencies
- Scope of each CSS module
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | ||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | ||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Consider using a CDN proxy or self-hosting for production
Using unpkg.com directly in production can be risky:
- Single point of failure
- Rate limiting concerns
- Potential security risks
Consider:
- Setting up a CDN proxy
- Self-hosting these assets
- Implementing a fallback mechanism
* added serving facility * form composer action bar fix * plan inbox assignee fix * formula and assumption refresh issue * action bar hidden fixes, session fixes * fixes * fix --------- Co-authored-by: NabeelAyubee <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js
(4 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
(19 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js (1)
Pattern **/*.js
: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (1)
Pattern **/*.js
: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js
[error] 23-23: Avoid unnecessary callback in flatMap call.
You can just use flat to flatten the array.
Safe fix: Replace unnecessary flatMap call to flat instead.
(lint/correctness/noFlatMapIdentity)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js
[error] 98-98: 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] 99-108: 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] 111-111: 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] 120-120: 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] 121-121: 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] 122-122: 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] 222-222: 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] 227-227: 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] 228-242: 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] 243-243: 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] 244-246: 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] 247-266: 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] 267-280: 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] 272-272: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 191-191: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 195-220: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 225-225: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 311-311: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 312-312: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 343-343: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 222-222: This let declares a variable that is only assigned once.
'epoch' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 459-459: 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] 472-472: 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] 776-776: 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] 791-791: 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)
🔇 Additional comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js (2)
Line range hint 43-64
: Verify security implications of new route configurations.
The new routes for microplan setup and boundary management need security review:
- Ensure proper access control on the backend for these routes
- Verify that status parameter in the microplan URL is properly sanitized
- Consider adding route guards for additional security
✅ Verification successful
Based on the shell script results, I can provide a final response as the verification is complete. The results show:
- The codebase uses
PrivateRoute
component from@egovernments/digit-ui-react-components
consistently across all routes, including the campaign manager routes. - The microplan status parameter is a controlled value set as a constant
RESOURCE_ESTIMATIONS_APPROVED
and is only used for navigation purposes. - All campaign manager routes are properly protected with
PrivateRoute
which handles authentication and role-based access control.
Route configurations are properly secured with appropriate access controls
- The
PrivateRoute
component inreact-components/src/atoms/PrivateRoute.js
handles authentication and role-based access - The microplan status parameter is a controlled constant value, mitigating URL parameter manipulation risks
- All campaign manager routes, including the new ones, are consistently protected with
PrivateRoute
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for route protection patterns
rg "RouteGuard|PrivateRoute|isAuthenticated" --type js
# Search for URL parameter handling
rg "microplanStatus|status=" --type js
Length of output: 28740
8-10
: Review role-based access control implementation.
The role definitions have been expanded to include new roles. Please ensure:
- These roles align with the backend authorization system
- The grouping of roles (especially combining "CAMPAIGN_MANAGER" and "MICROPLAN_CAMPAIGN_INTEGRATOR") follows the principle of least privilege
✅ Verification successful
Role definitions are consistently implemented across components
The role definitions and their usage are identical in both CampaignCard.js and OldCampaignCard.js, with the only difference being the link paths. The grouping of "CAMPAIGN_MANAGER" with "MICROPLAN_CAMPAIGN_INTEGRATOR" is consistently applied, suggesting this is an intentional design pattern rather than a security concern. The BOUNDARY_MANAGER role is also consistently used for boundary management functionality in both components.
- Both files implement identical ROLES object structure
- BOUNDARY_MANAGER role is properly isolated to boundary management functionality
- Role combinations are consistently applied across components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for role usage consistency across the codebase
rg -l "MICROPLAN_CAMPAIGN_INTEGRATOR|BOUNDARY_MANAGER" --type js
Length of output: 270
Script:
#!/bin/bash
# Check the role usage in both files to compare implementations
echo "=== CampaignCard.js roles ==="
rg "MICROPLAN_CAMPAIGN_INTEGRATOR|BOUNDARY_MANAGER" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignCard.js -B 2 -A 2
echo -e "\n=== OldCampaignCard.js roles ==="
rg "MICROPLAN_CAMPAIGN_INTEGRATOR|BOUNDARY_MANAGER" health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/OldCampaignCard.js -B 2 -A 2
# Check if these roles are defined/used in any configuration files
echo -e "\n=== Checking config files ==="
fd -e js -e json "config|constants" --exec rg "MICROPLAN_CAMPAIGN_INTEGRATOR|BOUNDARY_MANAGER" {} || true
Length of output: 1483
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/configs/UICustomizations.js (2)
20-39
: Function updateServiceDefinition
is implemented correctly
The function correctly updates the service definition and handles errors appropriately.
40-60
: Function retryCampaign
is implemented correctly
The logic for retrying a campaign is properly implemented and handles cases where the campaign is found.
// const { isLoading, data } = Digit.Hooks.useCustomAPIHook( | ||
// reqCriteria | ||
// ); | ||
const microplanStatus = "RESOURCE_ESTIMATIONS_APPROVED" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Move status constant to configuration file.
The hardcoded microplan status should be moved to a configuration file for better maintainability and reusability.
Create a new constant in your campaign configuration file:
// campaignConfig.js
export const MICROPLAN_STATUSES = {
RESOURCE_ESTIMATIONS_APPROVED: "RESOURCE_ESTIMATIONS_APPROVED",
// Add other statuses here
};
const propsForModuleCard = { | ||
Icon: <SVG.Support fill="white" height="36" width="36"/>, | ||
Icon: "Engineering", | ||
moduleName: t("ACTION_TEST_CAMPAIGN"), | ||
kpis: [], | ||
links: links, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Icon property needs to be a React component, not a string
Looking at the EmployeeModuleCard implementation, the Icon prop is directly rendered within a span element: <span className="logo removeBorderRadiusLogo">{Icon}</span>
. The string value "Engineering" will not work as expected since:
- The component expects Icon to be a React component that can be rendered
- There's no string-to-icon conversion mechanism in the implementation
- Other usages in the codebase import and pass actual icon components (e.g., WorksMgmtIcon)
- Change
Icon: "Engineering"
to use the appropriate icon component from your component library
🔗 Analysis chain
Verify Icon property compatibility.
The Icon property has been changed from a component to a string value "Engineering". Ensure that:
- The EmployeeModuleCard component supports string values for icons
- "Engineering" is a valid icon identifier in your icon system
Let me gather more information about the icon system and its usage.
Let me try one more search to understand how the Icon prop is used in the EmployeeModuleCard component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for EmployeeModuleCard usage patterns
rg "EmployeeModuleCard.*Icon" --type js -A 5
Length of output: 2661
Script:
#!/bin/bash
# Search for icon imports and usage in the codebase
rg "Engineering.*Icon" --type js
# Check how icons are used in EmployeeModuleCard component
ast-grep --pattern 'const EmployeeModuleCard = { Icon, $$$_ } => {
$$$
return (
$$$
<$_Icon$_/>
$$$
)
}'
Length of output: 159
Script:
#!/bin/bash
# Get the implementation of EmployeeModuleCard to see how Icon is rendered
cat micro-ui/web/micro-ui-internals/packages/react-components/src/atoms/EmployeeModuleCard.js
# Search for any string-based icon usage patterns
rg "Icon=\"[^\"]+\"" --type js
Length of output: 3911
userId&&(data.body.PlanConfigurationSearchCriteria.userUuid = userId); | ||
status&&(data.body.PlanConfigurationSearchCriteria.status = [status]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Refactor assignments within expressions for clarity
Using assignments inside expressions can reduce code readability. Consider refactoring these lines to use explicit if
statements.
Apply this diff to improve clarity:
- userId&&(data.body.PlanConfigurationSearchCriteria.userUuid = userId);
- status&&(data.body.PlanConfigurationSearchCriteria.status = [status]);
+ if (userId) {
+ data.body.PlanConfigurationSearchCriteria.userUuid = userId;
+ }
+ if (status) {
+ data.body.PlanConfigurationSearchCriteria.status = [status];
+ }
📝 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.
userId&&(data.body.PlanConfigurationSearchCriteria.userUuid = userId); | |
status&&(data.body.PlanConfigurationSearchCriteria.status = [status]); | |
if (userId) { | |
data.body.PlanConfigurationSearchCriteria.userUuid = userId; | |
} | |
if (status) { | |
data.body.PlanConfigurationSearchCriteria.status = [status]; | |
} |
🧰 Tools
🪛 Biome (1.9.4)
[error] 311-311: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 312-312: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
* some fixes * Update PlanInbox.js
* added checklist redirection * removed console
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Response.js (1)
Line range hint
57-67
: Remove commented codeRemove the commented-out code block if it's no longer needed. If this code might be needed later, consider documenting the reason in a TODO comment or tracking it in version control.
🧰 Tools
🪛 Biome (1.9.4)
[error] 55-55: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Response.js
(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Response.js (1)
Pattern **/*.js
: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Response.js
[error] 70-70: 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] 71-71: 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 (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Response.js (1)
Line range hint 1-82
: Verify error handling and loading states
Please ensure the component properly handles:
- Network errors during navigation
- Invalid state or queryString parameters
- Loading states during the timeout period
✅ Verification successful
Based on the search results, I can see that:
- There is an ErrorBoundary component in the core module that handles React errors and provides fallback UI.
- The Response component in question is a simple presentation component that shows success/error messages and action buttons.
- The component already has basic error handling through the isResponseSuccess state.
Let me verify the loading state handling with another search:
Based on the search results, I can see that:
- The Response component is wrapped in an ErrorBoundary component at the module level (from Module.js)
- The component has proper error state handling through isResponseSuccess
- The component has a timeout-based loading state during navigation (2000ms timeout)
- The parent components handle loading states with Loader components
- Invalid query parameters are handled with default values (e.g.,
queryStrings?.isSuccess === "true" ? true : queryStrings?.isSuccess === "false" ? false : true
)
Let me verify one final aspect - the navigation error handling:
The component has proper error handling and loading states in place
The Response component has comprehensive error and loading state handling:
-
Error handling:
- Wrapped in ErrorBoundary at module level
- Uses isResponseSuccess state with proper fallback
- Navigation errors handled through history API with fallback paths
-
Loading states:
- Implements 2000ms timeout during navigation with cleanup
- Parent components show Loader during data fetching
- Handles undefined/invalid query parameters with defaults
-
Navigation:
- All navigation calls have fallback paths
- Uses proper state cleanup in useEffect
- Secondary actions have proper error handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for error boundary implementations and loading states
ast-grep --pattern 'class ErrorBoundary extends React.Component {
$$$
}'
# Search for loading state handling
rg -l "isLoading|loading" --type js
Length of output: 31811
Script:
#!/bin/bash
# Search for loading state handling in the Response component and its parent components
rg -A 5 "isLoading|loading" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/
Length of output: 73863
Script:
#!/bin/bash
# Search for error handling in navigation and history usage
rg -A 5 "history.push|navigate" health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/Response.js
Length of output: 1062
@@ -1,8 +1,8 @@ | |||
import React, { useState, Fragment } from "react"; | |||
import React, { useState, Fragment , useEffect} from "react"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Clean up import statements
- Remove the empty import statement
- Consider consolidating component imports if both UI libraries are needed
import React, { useState, Fragment, useEffect } from "react";
-import { } from "@egovernments/digit-ui-react-components";
-import { PanelCard, ActionBar, Button } from "@egovernments/digit-ui-components";
+import { PanelCard, ActionBar, Button } from "@egovernments/digit-ui-components";
Also applies to: 4-5
const navigate = (link) => { | ||
history.push(link ? link : `/${window.contextPath}/employee/`); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Enhance path construction in navigate function
Consider using a utility function for consistent path construction and handling edge cases.
const navigate = (link) => {
- history.push(link ? link : `/${window.contextPath}/employee/`);
+ const defaultPath = `/${window.contextPath}/employee/`;
+ const sanitizedLink = link?.trim() || defaultPath;
+ history.push(sanitizedLink);
};
📝 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 navigate = (link) => { | |
history.push(link ? link : `/${window.contextPath}/employee/`); | |
const navigate = (link) => { | |
const defaultPath = `/${window.contextPath}/employee/`; | |
const sanitizedLink = link?.trim() || defaultPath; | |
history.push(sanitizedLink); | |
useEffect(() => { | ||
if (state?.actionLabel === "HCM_CONFIGURE_APP_RESPONSE_ACTION" && queryStrings?.isSuccess === "true") { | ||
const timer = setTimeout(() => { | ||
navigate(state?.actionLink || `/${window.contextPath}/employee/`); | ||
}, 2000); | ||
|
||
return () => clearTimeout(timer); | ||
} | ||
}, [state?.actionLabel, state?.actionLink]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Extract timeout duration as a constant
The timeout implementation looks good, but the magic number 2000 should be extracted as a named constant for better maintainability.
+const NAVIGATION_DELAY_MS = 2000;
useEffect(() => {
if (state?.actionLabel === "HCM_CONFIGURE_APP_RESPONSE_ACTION" && queryStrings?.isSuccess === "true") {
const timer = setTimeout(() => {
- navigate(state?.actionLink || `/${window.contextPath}/employee/`);
+ navigate(state?.actionLink);
- }, 2000);
+ }, NAVIGATION_DELAY_MS);
return () => clearTimeout(timer);
}
}, [state?.actionLabel, state?.actionLink]);
Committable suggestion skipped: line range outside the PR's diff.
<ActionBar | ||
actionFields={[ | ||
<Button label={state?.secondaryActionLabel ? t(state?.secondaryActionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")} onClick={()=>navigate(state?.secondaryActionLink)} type="button" variation="secondary"/>, | ||
<Button icon="ArrowForward" isSuffix label={state?.actionLabel ? t(state?.actionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")} onClick={()=>navigate(state?.actionLink)} type="button"/> | ||
]} | ||
className="" | ||
maxActionFieldsAllowed={5} | ||
setactionFieldsToRight | ||
sortActionFields | ||
style={{}} | ||
/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clean up ActionBar implementation
Several improvements needed in the ActionBar implementation:
- Add key props to button elements (required by React)
- Remove empty style and className props
- Consider if maxActionFieldsAllowed is necessary
- Document the purpose of setactionFieldsToRight and sortActionFields
<ActionBar
actionFields={[
- <Button label={state?.secondaryActionLabel ? t(state?.secondaryActionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")} onClick={() => navigate(state?.secondaryActionLink)} type="button" variation="secondary"/>,
- <Button icon="ArrowForward" isSuffix label={state?.actionLabel ? t(state?.actionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")} onClick={() => navigate(state?.actionLink)} type="button"/>
+ <Button
+ key="secondary-action"
+ label={state?.secondaryActionLabel ? t(state?.secondaryActionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")}
+ onClick={() => navigate(state?.secondaryActionLink)}
+ type="button"
+ variation="secondary"
+ />,
+ <Button
+ key="primary-action"
+ icon="ArrowForward"
+ isSuffix
+ label={state?.actionLabel ? t(state?.actionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")}
+ onClick={() => navigate(state?.actionLink)}
+ type="button"
+ />
]}
- className=""
- maxActionFieldsAllowed={5}
setactionFieldsToRight
sortActionFields
- style={{}}
/>
📝 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.
<ActionBar | |
actionFields={[ | |
<Button label={state?.secondaryActionLabel ? t(state?.secondaryActionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")} onClick={()=>navigate(state?.secondaryActionLink)} type="button" variation="secondary"/>, | |
<Button icon="ArrowForward" isSuffix label={state?.actionLabel ? t(state?.actionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")} onClick={()=>navigate(state?.actionLink)} type="button"/> | |
]} | |
className="" | |
maxActionFieldsAllowed={5} | |
setactionFieldsToRight | |
sortActionFields | |
style={{}} | |
/> | |
<ActionBar | |
actionFields={[ | |
<Button | |
key="secondary-action" | |
label={state?.secondaryActionLabel ? t(state?.secondaryActionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")} | |
onClick={() => navigate(state?.secondaryActionLink)} | |
type="button" | |
variation="secondary" | |
/>, | |
<Button | |
key="primary-action" | |
icon="ArrowForward" | |
isSuffix | |
label={state?.actionLabel ? t(state?.actionLabel) : t("ES_CAMPAIGN_RESPONSE_ACTION")} | |
onClick={() => navigate(state?.actionLink)} | |
type="button" | |
/> | |
]} | |
setactionFieldsToRight | |
sortActionFields | |
/> |
🧰 Tools
🪛 Biome (1.9.4)
[error] 70-70: 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] 71-71: 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)
* fixed pop inbox boundary getting empty issue * fixed back button issue * updated search juridiction component * fixed facility pop up boundary refresh * removed pending for approval from status filter --------- Co-authored-by: rachana-egov <[email protected]>
* updated table and other fixes * fixed table issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
Line range hint
13-31
: Consider cleaning up commented codeThe file contains several commented-out stylesheet and script includes. Consider removing these if they're no longer needed, or document why they're kept for reference.
- <!-- added below css for hcm-workbench module inclusion--> - <!-- <link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> --> - - <!-- <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css/dist/index.css"/> --> - <!-- <script src="https://s3.ap-south-1.amazonaws.com/egov-dev-assets/globalConfigs.js"></script> -->
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (2)
health/micro-ui/web/micro-ui-internals/example/package.json
is excluded by!**/*.json
health/micro-ui/web/micro-ui-internals/packages/css/package.json
is excluded by!**/*.json
📒 Files selected for processing (2)
health/micro-ui/web/micro-ui-internals/example/public/index.html
(2 hunks)health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/microplanInbox.scss
(1 hunks)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
15-15
: Verify health CSS module dependencies
The addition of digit-ui-health-css
suggests a move towards feature-specific styling. Ensure that there are no style conflicts or duplicate definitions between the base CSS and this new health-specific module.
Let's check for potential style conflicts:
✅ Verification successful
No style conflicts detected between health-specific and base CSS
Based on the analysis of the codebase:
- The health-specific CSS module (
@egovernments/digit-ui-health-css
) is properly isolated in its own package - No conflicting class names found between health and base styles - health-related styles are properly namespaced
- The CSS imports are correctly ordered in the HTML file with base styles loaded before health-specific styles
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for common class names or style definitions that might conflict
rg -g '*.css' -g '*.scss' '.health-'
# Look for specific health-related class definitions
ast-grep --pattern 'health-$_'
Length of output: 69
Script:
#!/bin/bash
# Let's try a broader search to understand the CSS structure and potential conflicts
# Search for any CSS files in the project
fd -e css -e scss
# Check for any style imports in the codebase
rg -g '*.js' -g '*.jsx' -g '*.html' '@egovernments/digit-ui.*css'
# Look for any existing CSS class patterns that might conflict
rg -g '*.css' -g '*.scss' -g '*.less' '\.(health|digit)'
Length of output: 52664
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/microplanInbox.scss (1)
55-57
: 🛠️ Refactor suggestion
Avoid Using !important
for Maintainable Styles
The use of !important
in the .sc-fzXfPc
class can make the CSS harder to maintain and override when necessary. It's best practice to increase the specificity of your selectors or restructure your CSS to eliminate the need for !important
.
Apply this diff to remove the !important
flag:
.sc-fzXfPc {
- border-radius: 0rem !important;
+ border-radius: 0rem;
}
⛔ Skipped due to learnings
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#675
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:33-58
Timestamp: 2024-11-10T20:01:49.000Z
Learning: The use of `!important` in the `.wbh-header-container` and `.guideline-actionbar-content` CSS classes within `microplanning.scss` is necessary to prevent these styles from being overridden.
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#675
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:649-652
Timestamp: 2024-11-12T10:40:17.779Z
Learning: The use of `!important` in the `.excel-wrapper` CSS class is necessary to prevent the styles from being overridden in certain contexts.
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#876
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:86-97
Timestamp: 2024-11-10T20:01:49.000Z
Learning: The use of `!important` in the `.modal-header` CSS class within `microplanning.scss` is necessary to ensure the styles are applied as intended and are not overridden by other styles.
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#377
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:19-25
Timestamp: 2024-11-12T10:40:11.591Z
Learning: The use of `!important` in the `.api-data-loader` CSS class is necessary due to the component being used in multiple places which leads to its styles being overridden if `!important` is not used.
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | ||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | ||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Based on the npm registry data, I can now provide a final response:
Version stability concerns confirmed - consider updating dependencies
The current dependency versions show varying levels of stability:
[email protected]
- Using a stable release (latest is 1.8.10)[email protected]
- Still in early beta stage with frequent breaking changes[email protected]
- Recently released package with rapid version increments
Recommend:
- Update
digit-ui-css
to latest stable 1.8.10 for bug fixes - Exercise caution with beta dependencies - consider pinning
digit-ui-components-css
to a specific beta version until stable release - Monitor
digit-ui-health-css
updates closely as it's under active development
🔗 Analysis chain
Review version stability and dependency management
While the update moves towards more stable versions for the base CSS (1.8.3), both digit-ui-components-css (beta.41) and digit-ui-health-css (0.1.x) are still in pre-release versions. This could lead to breaking changes in future updates.
Let's verify the version stability and release status:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check release information for the packages
for pkg in "@egovernments/digit-ui-css" "@egovernments/digit-ui-components-css" "@egovernments/digit-ui-health-css"; do
echo "Checking $pkg..."
curl -s "https://registry.npmjs.org/$pkg" | jq -r '.["dist-tags"], ."time"'
done
Length of output: 39489
.rdt_TableRow:hover > .rdt_TableCell:first-of-type { | ||
position: sticky; | ||
z-index: 10; | ||
left: 0rem; | ||
background-color: #FBEEE8; | ||
} | ||
|
||
|
||
.rdt_TableRow.selectedRow > .rdt_TableCell:first-of-type { | ||
position: sticky; | ||
z-index: 10; | ||
left: 0rem; | ||
background-color: #FBEEE8; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Refactor Repetitive Styles to Adhere to DRY Principle
The styles for sticky positioning and background colors are repeated across multiple selectors for hovered and selected rows. To enhance maintainability and follow the DRY (Don't Repeat Yourself) principle, consider extracting these common styles into reusable classes or mixins.
Here's how you could refactor the code:
Define reusable classes:
/* Define at the top of your stylesheet */
.sticky-first-cell {
position: sticky;
z-index: 10;
left: 0rem;
}
.highlighted-cell {
background-color: #FBEEE8;
}
Apply the reusable classes:
-.rdt_TableRow:hover > .rdt_TableCell:first-of-type {
- position: sticky;
- z-index: 10;
- left: 0rem;
- background-color: #FBEEE8;
-}
+ .rdt_TableRow:hover > .rdt_TableCell:first-of-type {
+ @extend .sticky-first-cell;
+ @extend .highlighted-cell;
+ }
-.rdt_TableRow.selectedRow > .rdt_TableCell:first-of-type {
- position: sticky;
- z-index: 10;
- left: 0rem;
- background-color: #FBEEE8;
-}
+ .rdt_TableRow.selectedRow > .rdt_TableCell:first-of-type {
+ @extend .sticky-first-cell;
+ @extend .highlighted-cell;
+ }
Repeat this pattern for the selectors within .microPlanBulkTable .selectable
to reduce code duplication.
Also applies to: 117-131
.rdt_TableHead{ | ||
z-index: 13 !important; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Use Variables for Consistent z-index
Management
Multiple hard-coded z-index
values (10
, 12
, 13
) are used throughout the styles. Defining these values as variables improves readability and makes it easier to manage stacking contexts across your application.
Define z-index
variables:
/* At the top of your stylesheet */
$z-index-base: 10;
$z-index-header: 12;
$z-index-table-head: 13;
Apply the variables:
-.rdt_TableHead {
- z-index: 13 !important;
-}
+.rdt_TableHead {
+ z-index: $z-index-table-head !important;
+}
-.rdt_TableRow > .rdt_TableCell:first-of-type {
- z-index: 10;
-}
+.rdt_TableRow > .rdt_TableCell:first-of-type {
+ z-index: $z-index-base;
+}
-.rdt_TableHeadRow > .rdt_TableCol:first-of-type {
- z-index: 12;
-}
+.rdt_TableHeadRow > .rdt_TableCol:first-of-type {
+ z-index: $z-index-header;
+}
This approach ensures consistency and simplifies future adjustments to the stacking order.
Also applies to: 100-106, 132-138
* Popup changed to alert type * UserName to name * userName to name * changes to heading font wieght * css package update * Removed i icon * changes * removed comments * removed comments * changes * isLoading removed * updated css --------- Co-authored-by: Nipun Arora <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
health/micro-ui/web/micro-ui-internals/packages/css/package.json
is excluded by!**/*.json
📒 Files selected for processing (2)
health/micro-ui/web/micro-ui-internals/example/public/index.html
(2 hunks)health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss
(1 hunks)
🧰 Additional context used
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss (1)
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-11-10T20:01:49.000Z
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 (5)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
15-15
: Verify integration with the new health-specific CSS module
The addition of digit-ui-health-css
suggests a move towards feature-specific styling. Please verify:
- No style conflicts with base CSS
- Health-specific components render correctly
- No duplicate style definitions
✅ Verification successful
Let me try another approach to gather information about CSS usage and potential conflicts.
Let me try one more search to specifically look at the health CSS module's usage and any potential conflicts.
Based on the search results, I can now provide a final response:
Health CSS module integration appears properly configured
The health-specific CSS module (@egovernments/digit-ui-health-css) is:
- Correctly imported in the main index.html
- Using a consistent version (0.1.42) across the application
- No conflicting CSS imports are present (other CSS modules are commented out)
- The module is properly scoped within the health domain
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential CSS conflicts and duplicates
# Search for health-specific style definitions in other CSS modules
rg -l "health-" --type css
# Look for potential conflicting class names
for file in $(fd -e css); do
echo "=== $file ==="
rg -o '\.[\w-]+\s*{' "$file" | sort | uniq -d
done
Length of output: 2235
Script:
#!/bin/bash
# Search for health-specific CSS classes and their usage
rg -l "\.health-" --type css
# Check for any CSS imports in health-related components
rg -l "import.*\.css" health/
# Look for style-related attributes in health components
rg "className=" health/
# Check package.json for CSS-related dependencies
fd package.json --exec cat {} | jq -r '.dependencies | keys[]' | grep -i css
Length of output: 104483
Script:
#!/bin/bash
# Check for health-specific CSS imports and classes
rg -A 5 "@egovernments/digit-ui-health-css"
# Look for any potential style overrides
rg -l "\.health-.*{" --type css
# Check package.json for the health CSS module version
rg -l "digit-ui-health-css" health/micro-ui/web/package.json
Length of output: 4404
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss (4)
373-378
: 🧹 Nitpick (assertive)
Standardize spacing units
The gap property uses inconsistent units (0rem vs 24px elsewhere).
Apply this change:
.fac-kpi-container {
display: flex;
justify-content: space-between;
- gap: 0rem;
+ gap: 1.5rem;
align-items: center;
}
Likely invalid or redundant comment.
248-253
: 🧹 Nitpick (assertive)
Remove !important declarations
The selected row styles use !important
flags which can lead to specificity issues.
Increase selector specificity instead:
- .rdt_TableRow {
- &.selectedRow {
- background-color: #FBEEE8 !important;
- border-bottom-color: #d6d5d4 !important;
- }
- }
+ table .rdt_TableRow.selectedRow {
+ background-color: theme(digitv2.lightTheme.selectedBackground);
+ border-bottom-color: theme(digitv2.lightTheme.border);
+ }
Likely invalid or redundant comment.
127-160
: 🛠️ Refactor suggestion
Improve table layout and accessibility
The table-like structure has potential issues:
- Fixed table layout with hardcoded widths may cause layout issues
- Text truncation without proper accessibility considerations
Consider these improvements:
.as-table-like {
display: table;
width: 100%;
border-collapse: collapse;
- table-layout: fixed;
+ table-layout: auto;
}
.as-key-cell {
font-weight: bold;
- min-width: 20rem;
+ width: 30%;
background-color: #fff;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
+ &[title] {
+ cursor: help;
+ }
}
.as-value-cell {
- width: 70%;
+ width: auto;
overflow: hidden;
text-overflow: ellipsis;
white-space: nowrap;
+ &[title] {
+ cursor: help;
+ }
}
Likely invalid or redundant comment.
181-188
:
Fix syntax error and improve grid layout
There's a syntax error in the border-bottom property and the comment is misaligned.
Apply this fix:
.as-label-field {
display: grid;
grid-template-columns: 1fr 2fr;
gap: 1rem;
overflow: auto;
align-items: center;
- border-bottom: '1px solid #e0e0e0' /* Align items vertically in the center */
+ border-bottom: 1px solid #e0e0e0; /* Add bottom border */
}
Likely invalid or redundant comment.
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | ||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> | ||
<link rel="stylesheet" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Positive: Improved version stability with consolidated dependencies
Good improvements in dependency management:
- Consolidated multiple CSS versions into a single stable version (1.8.3)
- Removed alpha/pre-release versions
- Organized health-specific styles into a dedicated module
However, note that digit-ui-components-css
is still in beta (0.0.2-beta.41). Consider:
- Documenting any potential breaking changes when upgrading from beta versions
- Planning for migration to stable releases when available
.activity-grid { | ||
display: grid; | ||
grid-template-columns: 33.3% 33.3% 33.3%; | ||
text-align: -webkit-center; | ||
gap: 24px; | ||
margin-right: 3rem; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Fix grid layout implementation
The activity grid implementation has several issues:
- Fixed percentage columns don't account for gaps
- Unnecessary vendor prefix for text-align
- Hardcoded margin
Apply these changes:
.activity-grid {
display: grid;
- grid-template-columns: 33.3% 33.3% 33.3%;
+ grid-template-columns: repeat(3, 1fr);
- text-align: -webkit-center;
+ text-align: center;
gap: 24px;
- margin-right: 3rem;
+ padding-right: 3rem;
}
📝 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.
.activity-grid { | |
display: grid; | |
grid-template-columns: 33.3% 33.3% 33.3%; | |
text-align: -webkit-center; | |
gap: 24px; | |
margin-right: 3rem; | |
} | |
.activity-grid { | |
display: grid; | |
grid-template-columns: repeat(3, 1fr); | |
text-align: center; | |
gap: 24px; | |
padding-right: 3rem; | |
} |
.role-summary-sub-heading { | ||
|
||
color: theme(digitv2.lightTheme.primary-2) !important; | ||
margin-bottom: 0rem; | ||
font-size: 1rem; | ||
display:flex; | ||
justify-content: space-between; | ||
|
||
|
||
|
||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Clean up empty lines and standardize spacing
The role-summary-sub-heading class contains unnecessary empty lines.
Apply this change:
.role-summary-sub-heading {
color: theme(digitv2.lightTheme.primary-2) !important;
margin-bottom: 0rem;
font-size: 1rem;
display: flex;
justify-content: space-between;
-
-
-
-
}
📝 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.
.role-summary-sub-heading { | |
color: theme(digitv2.lightTheme.primary-2) !important; | |
margin-bottom: 0rem; | |
font-size: 1rem; | |
display:flex; | |
justify-content: space-between; | |
} | |
.role-summary-sub-heading { | |
color: theme(digitv2.lightTheme.primary-2) !important; | |
margin-bottom: 0rem; | |
font-size: 1rem; | |
display: flex; | |
justify-content: space-between; | |
} |
.pagination-controls { | ||
display: flex; | ||
justify-content: space-between; | ||
align-items: center; | ||
padding: 1rem; | ||
border-top: 1px solid #ccc; | ||
margin-top: 1rem; | ||
font-size: 0.9rem; | ||
} | ||
|
||
.pagination-controls label { | ||
display: flex; | ||
align-items: center; | ||
gap: 0.5rem; | ||
} | ||
|
||
.pagination-controls select { | ||
padding: 0.3rem; | ||
font-size: 0.9rem; | ||
border-radius: 4px; | ||
border: 1px solid #ccc; | ||
} | ||
|
||
.pagination-controls div { | ||
display: flex; | ||
align-items: center; | ||
gap: 0.5rem; | ||
} | ||
|
||
.pagination-controls button { | ||
background: none; | ||
border: none; | ||
color: #007bff; | ||
cursor: pointer; | ||
font-size: 0.9rem; | ||
} | ||
|
||
.pagination-controls button:disabled { | ||
color: #ccc; | ||
} | ||
|
||
.pagination-controls button:hover:not(:disabled) { | ||
text-decoration: underline; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Enhance pagination controls accessibility
The pagination controls lack proper ARIA attributes and keyboard interaction styles.
Add necessary accessibility attributes:
.pagination-controls {
+ nav[aria-label="Pagination"] {
display: flex;
justify-content: space-between;
align-items: center;
padding: 1rem;
border-top: 1px solid #ccc;
margin-top: 1rem;
font-size: 0.9rem;
}
button {
+ &:focus {
+ outline: 2px solid theme(digitv2.lightTheme.primary);
+ outline-offset: 2px;
+ }
}
+ [aria-current="page"] {
+ font-weight: bold;
+ text-decoration: underline;
+ }
}
Committable suggestion skipped: line range outside the PR's diff.
.search-tabs-container { | ||
display: flex; | ||
justify-content: space-between; | ||
background-color: #eee; | ||
|
||
.search-tab-head { | ||
padding: 10px 35px; | ||
font-weight: 700; | ||
font-size: 1rem; | ||
border: 1px solid #d6d5d4; | ||
border-radius: 0.5rem 0.5rem 0 0; | ||
} | ||
|
||
.search-tab-head-selected { | ||
padding: 10px 35px; | ||
color: rgb(244, 119, 56); | ||
background-color: #fff; | ||
border: 1px solid #f47738; | ||
border-radius: 0.5rem 0.5rem 0 0; | ||
border-bottom: 4px solid rgb(244, 119, 56); | ||
font-weight: 700; | ||
font-size: 1.125rem; | ||
margin-bottom: -1rem; | ||
} | ||
|
||
.search-tab-head-selected:focus { | ||
outline: 0; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Improve accessibility and consistency in search tabs
Several issues need to be addressed in the search tabs implementation:
- Removing focus outline (line 27) harms keyboard accessibility
- Inconsistent color formats (hex vs rgb)
- Hardcoded colors instead of theme variables
Apply these changes:
.search-tabs-container {
.search-tab-head-selected {
- color: rgb(244, 119, 56);
- border: 1px solid #f47738;
- border-bottom: 4px solid rgb(244, 119, 56);
+ color: theme(digitv2.lightTheme.primary);
+ border: 1px solid theme(digitv2.lightTheme.primary);
+ border-bottom: 4px solid theme(digitv2.lightTheme.primary);
}
.search-tab-head-selected:focus {
- outline: 0;
+ outline: 2px solid theme(digitv2.lightTheme.primary);
+ outline-offset: 2px;
}
}
📝 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.
.search-tabs-container { | |
display: flex; | |
justify-content: space-between; | |
background-color: #eee; | |
.search-tab-head { | |
padding: 10px 35px; | |
font-weight: 700; | |
font-size: 1rem; | |
border: 1px solid #d6d5d4; | |
border-radius: 0.5rem 0.5rem 0 0; | |
} | |
.search-tab-head-selected { | |
padding: 10px 35px; | |
color: rgb(244, 119, 56); | |
background-color: #fff; | |
border: 1px solid #f47738; | |
border-radius: 0.5rem 0.5rem 0 0; | |
border-bottom: 4px solid rgb(244, 119, 56); | |
font-weight: 700; | |
font-size: 1.125rem; | |
margin-bottom: -1rem; | |
} | |
.search-tab-head-selected:focus { | |
outline: 0; | |
} | |
} | |
.search-tabs-container { | |
display: flex; | |
justify-content: space-between; | |
background-color: #eee; | |
.search-tab-head { | |
padding: 10px 35px; | |
font-weight: 700; | |
font-size: 1rem; | |
border: 1px solid #d6d5d4; | |
border-radius: 0.5rem 0.5rem 0 0; | |
} | |
.search-tab-head-selected { | |
padding: 10px 35px; | |
color: theme(digitv2.lightTheme.primary); | |
background-color: #fff; | |
border: 1px solid theme(digitv2.lightTheme.primary); | |
border-radius: 0.5rem 0.5rem 0 0; | |
border-bottom: 4px solid theme(digitv2.lightTheme.primary); | |
font-weight: 700; | |
font-size: 1.125rem; | |
margin-bottom: -1rem; | |
} | |
.search-tab-head-selected:focus { | |
outline: 2px solid theme(digitv2.lightTheme.primary); | |
outline-offset: 2px; | |
} | |
} |
Co-authored-by: rachana-egov <[email protected]>
Co-authored-by: rachana-egov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/fetchFromMicroplan.js
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/fetchFromMicroplan.js (1)
Pattern **/*.js
: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/fetchFromMicroplan.js
[error] 92-92: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🔇 Additional comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/fetchFromMicroplan.js (1)
92-92
: 🧹 Nitpick (assertive)
Fix self-closing tag.
Use self-closing tag for empty div:
-<div className="sandbox-loader"></div>
+<div className="sandbox-loader" />
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 92-92: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
useEffect(() => { | ||
if (showToast) { | ||
setTimeout(closeToast, 3500); | ||
} | ||
}, [showToast]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Extract toast configuration to constants.
The toast timeout is hardcoded. Consider making it configurable:
+const TOAST_DURATION = 3500;
+
useEffect(() => {
if (showToast) {
- setTimeout(closeToast, 3500);
+ setTimeout(closeToast, TOAST_DURATION);
}
}, [showToast]);
📝 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.
useEffect(() => { | |
if (showToast) { | |
setTimeout(closeToast, 3500); | |
} | |
}, [showToast]); | |
const TOAST_DURATION = 3500; | |
useEffect(() => { | |
if (showToast) { | |
setTimeout(closeToast, TOAST_DURATION); | |
} | |
}, [showToast]); |
}, | ||
}; | ||
|
||
const { isLoading, data: resourceData, isFetching } = Digit.Hooks.useCustomAPIHook(reqCriteriaResource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve loading state handling.
The component fetches data but doesn't show a proper loading state or error details:
-const { isLoading, data: resourceData, isFetching } = Digit.Hooks.useCustomAPIHook(reqCriteriaResource);
+const {
+ isLoading,
+ data: resourceData,
+ isFetching,
+ error
+} = Digit.Hooks.useCustomAPIHook(reqCriteriaResource);
+
+useEffect(() => {
+ if (error) {
+ setShowToast({
+ key: "error",
+ label: t("SOME_ERROR_OCCURRED_IN_FETCH"),
+ message: error.message
+ });
+ }
+}, [error]);
Committable suggestion skipped: line range outside the PR's diff.
const steps = [ | ||
"FETCHING_CAMPAIGN_DATA_FROM_MICROPLAN", | ||
"FETCHING_CAMPAIGN_TYPE_FROM_MICROPLAN", | ||
"FETCHING_CAMPAIGN_TARGET_FROM_MICROPLAN", | ||
"FILLING_CAMPAIGN_TARGET_DATA_FROM_MICROPLAN", | ||
"FETCHING_CAMPAIGN_FACILITY_FROM_MICROPLAN", | ||
"FILLING_CAMPAIGN_FACILITY_DATA_FROM_MICROPLAN", | ||
"FETCHING_CAMPAIGN_USER_FROM_MICROPLAN", | ||
"FILLING_CAMPAIGN_USER_DATA_FROM_MICROPLAN", | ||
"UPDATED_CAMPAIGN_WITH_UPLODAED_DATA", | ||
"CMN_ALL_DATA_FETCH_DONE", | ||
]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Move steps array to a constants file.
Consider extracting the steps array to a dedicated constants file for better maintainability and reusability:
+// campaignConstants.js
+export const MICROPLAN_FETCH_STEPS = [
+ "FETCHING_CAMPAIGN_DATA_FROM_MICROPLAN",
+ // ... other steps
+];
+// fetchFromMicroplan.js
+import { MICROPLAN_FETCH_STEPS } from "./campaignConstants";
-const steps = [ ... ];
+const steps = MICROPLAN_FETCH_STEPS;
Committable suggestion skipped: line range outside the PR's diff.
"UPDATED_CAMPAIGN_WITH_UPLODAED_DATA", | ||
"CMN_ALL_DATA_FETCH_DONE", | ||
]; | ||
const tenantId = Digit.ULBService.getCurrentTenantId(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add validation for tenantId.
The tenantId
is retrieved but not validated. Add validation to prevent potential issues:
-const tenantId = Digit.ULBService.getCurrentTenantId();
+const tenantId = Digit.ULBService.getCurrentTenantId();
+if (!tenantId) {
+ setShowToast({ key: "error", label: t("ERR_TENANT_ID_REQUIRED") });
+ return;
+}
Committable suggestion skipped: line range outside the PR's diff.
{showToast && ( | ||
<Toast | ||
type={showToast?.key === "error" ? "error" : showToast?.key === "info" ? "info" : "success"} | ||
label={t(showToast?.label)} | ||
onClose={closeToast} | ||
/> | ||
)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Extract Toast component.
Consider extracting the Toast logic into a reusable component:
+const StatusToast = ({ toast, onClose }) => {
+ if (!toast) return null;
+ return (
+ <Toast
+ type={toast?.key === "error" ? "error" : toast?.key === "info" ? "info" : "success"}
+ label={t(toast?.label)}
+ onClose={onClose}
+ />
+ );
+};
-{showToast && (
- <Toast
- type={showToast?.key === "error" ? "error" : showToast?.key === "info" ? "info" : "success"}
- label={t(showToast?.label)}
- onClose={closeToast}
- />
-)}
+<StatusToast toast={showToast} onClose={closeToast} />
📝 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.
{showToast && ( | |
<Toast | |
type={showToast?.key === "error" ? "error" : showToast?.key === "info" ? "info" : "success"} | |
label={t(showToast?.label)} | |
onClose={closeToast} | |
/> | |
)} | |
const StatusToast = ({ toast, onClose }) => { | |
if (!toast) return null; | |
return ( | |
<Toast | |
type={toast?.key === "error" ? "error" : toast?.key === "info" ? "info" : "success"} | |
label={t(toast?.label)} | |
onClose={onClose} | |
/> | |
); | |
}; | |
<StatusToast toast={showToast} onClose={closeToast} /> |
import React, { useEffect, useState } from "react"; | ||
import { CheckCircle } from "@egovernments/digit-ui-svg-components"; | ||
import { useHistory, useLocation } from "react-router-dom"; | ||
import { useTranslation } from "react-i18next"; | ||
import { Header } from "@egovernments/digit-ui-react-components"; | ||
import { Toast } from "@egovernments/digit-ui-components"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Organize imports and consider using a dedicated hook for URL parameters.
Group related imports together and consider extracting URL parameter handling into a custom hook:
import React, { useEffect, useState } from "react";
+import { useHistory, useLocation } from "react-router-dom";
+import { useTranslation } from "react-i18next";
+
import { CheckCircle } from "@egovernments/digit-ui-svg-components";
-import { useHistory, useLocation } from "react-router-dom";
-import { useTranslation } from "react-i18next";
import { Header } from "@egovernments/digit-ui-react-components";
import { Toast } from "@egovernments/digit-ui-components";
+const useURLParams = () => {
+ const location = useLocation();
+ const searchParams = new URLSearchParams(location.search);
+ return {
+ id: searchParams.get("id"),
+ planConfigurationId: searchParams.get("planConfigurationId"),
+ searchParams
+ };
+};
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 51
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
Line range hint
17-21
: Clean up commented codeConsider removing the commented-out CSS and script tags if they're no longer needed. If they serve a specific purpose (e.g., debugging), add a clear comment explaining why they're kept.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
health/micro-ui/web/micro-ui-internals/packages/css/package.json
is excluded by!**/*.json
📒 Files selected for processing (9)
health/micro-ui/web/micro-ui-internals/example/public/index.html
(2 hunks)health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss
(5 hunks)health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/coreOverride.scss
(3 hunks)health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss
(1 hunks)health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/index.scss
(9 hunks)health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/microplanInbox.scss
(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js
(11 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js
(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryRelationCreate.js
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js (1)
Pattern **/*.js
: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js (1)
Pattern **/*.js
: check
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryRelationCreate.js (1)
Pattern **/*.js
: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js
[error] 111-111: This let declares a variable that is only assigned once.
'attributes' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 229-229: 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)
[error] 261-261: 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] 324-324: This let declares a variable that is only assigned once.
'temp' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 428-428: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 437-437: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 442-442: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 462-462: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 484-484: Unsafe usage of optional chaining.
If it short-circuits with 'undefined' the evaluation will throw TypeError here:
(lint/correctness/noUnsafeOptionalChaining)
[error] 495-495: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 520-520: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 540-540: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 560-560: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 698-698: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 704-704: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js
[error] 68-71: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 102-102: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 139-139: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 308-308: 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] 299-305: This code is unreachable
... because either this statement ...
... or this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 622-622: 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)
[error] 735-735: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 768-768: 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)
[error] 803-803: 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)
[error] 838-838: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 952-962: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 103-103: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 108-117: 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] 118-130: 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] 372-372: Use let or const instead of var.
A variable declared with var is accessible in the whole body of the function. Thus, the variable can be accessed before its initialization and outside the block where it is declared.
See MDN web docs for more details.
Unsafe fix: Use 'let' instead.
(lint/style/noVar)
[error] 622-622: This let declares a variable that is only assigned once.
'checked' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryRelationCreate.js
[error] 308-308: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 454-454: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 460-460: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 462-462: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 467-467: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 479-479: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 503-503: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 509-509: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 525-525: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 584-584: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 586-586: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 607-607: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 613-613: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 23-23: This let declares a variable that is only assigned once.
'locale' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 27-27: This let declares a variable that is only assigned once.
'receivedData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 73-73: This let declares a variable that is only assigned once.
'flag' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 235-235: This let declares a variable that is only assigned once.
'errorString' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 489-489: 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] 534-542: 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] 543-560: 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] 589-589: 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] 634-642: 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] 643-662: 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 (60)
health/micro-ui/web/micro-ui-internals/example/public/index.html (2)
13-15
: Previous review comments still apply
The following concerns from previous reviews remain valid:
- Adding integrity hashes for CDN resources
- Performance optimizations for stylesheet loading
- CSS module organization
- CDN proxy/self-hosting recommendations
13-15
: Review version selection strategy
The current dependency versions raise some concerns:
[email protected]
is still in beta, which may introduce breaking changes[email protected]
is an early release version (< 1.0.0)
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss (1)
203-207
: LGTM! Clean and consistent preview component styling.
The flexbox layout with consistent spacing follows best practices.
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/microplanInbox.scss (1)
151-153
: Review tooltip positioning strategy
Static positioning for tooltips may cause issues with overflow and visibility when the parent container has overflow: hidden
. Consider using absolute positioning with a positioned ancestor.
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/index.scss (48)
3-3
: New SCSS imports are correctly added
The imports for hcmworkbench.scss
, villageView.scss
, and facility.scss
appropriately include the necessary styles for the updated components.
Also applies to: 5-6
12-14
: Resetting margins for .employee .ground-container
Setting margin: 0;
ensures that any default margins are removed, providing a consistent layout.
16-17
: Adjusting vertical positioning of .digit-sidebar
Setting top: 4.5rem;
aligns the sidebar correctly relative to the top of the page, which may be necessary due to header height adjustments.
20-23
: Styling adjustments for .user-profile
The added margin-top
and margin-left
position the user profile appropriately within the layout.
27-27
: Inconsistent unit usage in margin-left
The margin-left: 60px;
uses pixels, whereas other spacing properties use rem
. For consistency and scalability across different screen sizes, consider converting 60px
to rem
.
31-34
: Removing right margins and paddings in .employee-app-wrapper
Eliminating side margins and paddings helps in utilizing full available space, which can be beneficial for responsive designs.
38-40
: Adjusting paddings and margins in .ground-container
Removing padding and bottom margins ensures the container aligns properly with other elements.
43-47
: Spacing and layout adjustments in .ground-container.moduleCardWrapper
The modifications set consistent gaps and margins, enhancing the grid layout.
62-64
: Margin adjustments in .campaign-cycle-container
Adding top margin to .popup-header-fix
may improve the visual spacing in popup headers.
82-85
: Adjusting top margin for .digit-bread-crumb
Setting margin-top: 0rem;
aligns the breadcrumb with adjacent elements.
89-89
: Use of !important
and hardcoded color in .mandatory-span
Overuse of !important
can lead to specificity issues, and hardcoded colors reduce maintainability.
100-100
: Increasing z-index
in .actionBarClass
Setting z-index: 10;
ensures the action bar overlays other elements when necessary.
107-107
: Margin and width adjustments in .previous-button
These changes provide consistent spacing and sizing for navigation buttons.
112-121
: Styling for .microplan-bulk-upload
and nested elements
The flex layout and spacing adjustments enhance the alignment of bulk upload components.
124-127
: Applying typography styles to .uploader-sub-heading
Using @extend
with .typography.heading-xl
ensures consistent heading styles.
130-133
: Styling .summary-main-heading
with consistent typography
Applying heading styles and theme colors maintains visual consistency across headings.
137-139
: Use of !important
in .summary-sub-heading
color
Overuse of !important
can cause specificity issues.
151-156
: Customizing .vertical-stepper-card
dimensions
Adjusting height and margins tailors the stepper card to fit the design specifications.
157-162
: Adjusting .vertical-custom-height-stepper-card
Setting custom heights provides flexibility for different content lengths.
165-171
: Styling adjustments in .card-with-background
Marginal changes and color adjustments improve the appearance of cards with backgrounds.
176-181
: Defining .no-data-found
layout
Centering content enhances user experience when no data is available.
185-190
: Updating .search-tab-head
and .search-tabs-container
Styling changes improve the search tab's visual consistency.
196-242
: Enhancing .delivery-preview-card
and nested table styles
The adjustments provide better spacing, background colors, and borders for preview cards and tables.
258-259
: Consistent color notation for white
Using both #fff
and white
can lead to inconsistency.
283-290
: Styling .cycle-paragraph
Font properties and margins set for consistent presentation of cycle-related text.
309-313
: Custom styling of date input picker indicator
Modifying ::-webkit-calendar-picker-indicator
affects only WebKit browsers.
Line range hint 344-355
: Defining .product-tag-container
and related elements
Flex layouts and font adjustments improve the display of product tags.
359-359
: Center aligning text in .digit-employee-card-input.numeric
This enhances the readability of numeric inputs.
364-369
: Creating responsive grids in .in-between
Using grid layouts and media queries ensures responsiveness across devices.
374-383
: Adjustments in .setup-campaign
components
Padding and height adjustments refine the appearance of campaign setup elements.
391-409
: Styling .campaign-summary-container
and nested elements
Margin and gap adjustments enhance the layout of the campaign summary.
414-421
: Use of !important
in .digit-infobanner-wrap.error
Overuse of !important
can lead to specificity issues.
448-474
: Layout improvements in .add-new-product-container
Adjusting borders, backgrounds, and spacing enhances the user interface for adding new products.
477-479
: Padding adjustments in .page-padding-fix
Adjustments ensure consistent spacing across pages.
484-492
: Styling .addProductActionClass
and nested .submit-bar
Reversing flex direction and setting background colors improve button placements.
503-509
: Alignment in .loginFormStyleEmployee
Centering content enhances the login form's usability.
513-513
: Adding cursor pointer in .hover
This provides visual feedback to users when hovering over interactive elements.
528-545
: Styling .digit-popup-wrapper.boundaries-pop-module
Width and button arrangements improve the modal's appearance and usability.
551-572
: Enhancements in .campaign-type-alert-button
and .campaign-pop-module
Adjustments improve the visual hierarchy and user interaction in campaign pop-ups.
Line range hint 590-595
: Adjustments in .digit-toast-success
Ensuring the toast message is responsive and word-wrapped improves readability.
602-604
: Defining error state styling in .digit-dropdown-select.error
Consistent border styling for error states enhances user feedback.
623-623
: Styling error state in .employeeCard.employeeCard-override.card-error
Applying a red border indicates errors effectively.
626-637
: Consolidate duplicate styles for .digit-radio-options-wrap
There are duplicate style definitions for .digit-radio-options-wrap
. Consider consolidating them to avoid redundancy.
638-647
: Duplicate CSS blocks for .digit-radio-options-wrap
Consolidating these styles simplifies maintenance.
758-758
: Use of !important
in selectors
Overuse of !important
can lead to specificity issues.
812-816
: Styling for .parent
class
Setting color and font properties enhances the presentation of parent elements.
820-851
: Adjustments in .view-campaign-details-card
Margin and padding changes improve the layout and readability of campaign details.
852-855
: Updating .facility-mapping-config
styles
Adjusting margins for dropdowns enhances spacing consistency.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/CampaignSummary.js (1)
698-698
: 🛠️ Refactor suggestion
Add optional chaining to prevent errors when accessing cards
In the conditional block, you have optional chaining with updatedObject?.cards?.[1]
, but in the subsequent line, you access updatedObject.cards
without checking if it exists. To prevent potential errors, use optional chaining.
Apply this diff:
- updatedObject.cards.forEach((card) => {
+ updatedObject.cards?.forEach((card) => {
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 698-698: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryRelationCreate.js (4)
341-341
: Verify the unusually long transitionTime
value
The transitionTime
prop for the Toast component is set to 100000
milliseconds (100 seconds) at line 341. This seems excessively long for a notification. Please verify if this is intended; otherwise, consider reducing it to a more standard duration (e.g., 3000 milliseconds).
489-512
: Add key
prop to elements in the list rendering
When rendering lists using map
, each element should have a unique key
prop to help React optimize rendering. At line 489, the div
rendered inside the newBoundaryData.map
lacks a key
prop.
🧰 Tools
🪛 Biome (1.9.4)
[error] 503-503: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 509-509: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 489-489: 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)
634-642
: Add key
prop to elements in actionFields
array
Elements in the actionFields
array at lines 634-642 and 643-662 should have a key
prop to prevent potential issues during re-renders.
Also applies to: 643-662
🧰 Tools
🪛 Biome (1.9.4)
[error] 634-642: 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)
408-427
:
Avoid mutating state directly in the addParents
function
The addParents
function modifies items directly within the map
function, which can lead to unintended side effects since state in React should be treated as immutable. Instead, create new item objects to prevent direct mutations.
Apply this refactor:
const addParents = () => {
setNewBoundaryData((prevItems) => {
return prevItems.map((item, idx) => {
+ let newItem = { ...item };
newItem.boundaryType = item.boundaryType.trim();
if (idx === 0) {
if (newHierarchy) newItem.parentBoundaryType = null;
else {
if (boundaryData.length === 0) newItem.parentBoundaryType = null;
else newItem.parentBoundaryType = boundaryData[boundaryData.length - 1].boundaryType.trim();
}
}
if (idx > 0) {
newItem.parentBoundaryType = prevItems[idx - 1].boundaryType.trim();
}
- return item;
+ return newItem;
});
});
};
Likely invalid or redundant comment.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js (1)
299-305
:
Remove unreachable code in the reducer function
The code between lines 299-305 is unreachable due to the preceding return statement in line 297.
Apply this diff to remove the unreachable code:
return [
...state,
{ code: action?.payload?.[1]?.code, name: action?.payload?.[1]?.name, propsData: action.payload },
];
- // Check if the item already exists to prevent duplication
- if (state.some((e) => e.code === action.payload?.[1]?.code) || !action?.payload?.[1]?.type) {
- return state; // Return state unchanged if item is already in queue
- }
- return [
- ...state,
- { code: action?.payload?.[1]?.code, name: action?.payload?.[1]?.name, propsData: action.payload },
- ];
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 299-305: This code is unreachable
... because either this statement ...
... or this statement will return from the function beforehand
(lint/correctness/noUnreachable)
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/coreOverride.scss (2)
206-207
: Review the use of negative margins in .campaign-flow-stepper
Applying negative margins (margin-left: -5.625rem;
, margin-right: -6.875rem;
) may cause layout issues or unexpected overlapping, especially on different screen sizes. Ensure that this is intentional and test across various devices to confirm that it doesn't affect responsiveness negatively.
44-46
: 🧹 Nitpick (assertive)
Minimize the use of !important
in .link-label
styles
Employing !important
can make the CSS harder to maintain and override. Consider increasing the specificity of the .link-label
selector or restructuring your styles to achieve the desired effect without relying on !important
.
⛔ Skipped due to learnings
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#675
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:33-58
Timestamp: 2024-11-10T20:01:49.000Z
Learning: The use of `!important` in the `.wbh-header-container` and `.guideline-actionbar-content` CSS classes within `microplanning.scss` is necessary to prevent these styles from being overridden.
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#876
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:86-97
Timestamp: 2024-11-10T20:01:49.000Z
Learning: The use of `!important` in the `.modal-header` CSS class within `microplanning.scss` is necessary to ensure the styles are applied as intended and are not overridden by other styles.
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#675
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:649-652
Timestamp: 2024-11-12T10:40:17.779Z
Learning: The use of `!important` in the `.excel-wrapper` CSS class is necessary to prevent the styles from being overridden in certain contexts.
.stepper-card{ | ||
height: 39rem; | ||
margin-bottom: 1.5rem !important; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider dynamic height for stepper card.
The fixed height of 39rem might cause content overflow issues. Consider using min-height or max-height with overflow handling.
.stepper-card{
- height: 39rem;
+ min-height: 39rem;
+ max-height: max-content;
+ overflow-y: auto;
margin-bottom: 1.5rem !important;
}
📝 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.
.stepper-card{ | |
height: 39rem; | |
margin-bottom: 1.5rem !important; | |
} | |
.stepper-card{ | |
min-height: 39rem; | |
max-height: max-content; | |
overflow-y: auto; | |
margin-bottom: 1.5rem !important; | |
} |
.label-field-wrapper{ | ||
width: -webkit-fill-available; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Improve cross-browser compatibility.
The -webkit-fill-available
property has limited browser support. Consider using a more widely supported approach.
.name-container-label{
.label-field-wrapper{
- width: -webkit-fill-available;
+ width: 100%;
+ width: stretch;
}
}
Committable suggestion skipped: line range outside the PR's diff.
td{ | ||
background: rgba(#f47738, 0.12); | ||
cursor: pointer; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Add transition for smoother hover effect.
Consider adding a transition property to make the hover effect more polished.
tbody {
tr:hover {
td{
background: rgba(#f47738, 0.12);
cursor: pointer;
+ transition: background-color 0.2s ease;
}
}
}
Committable suggestion skipped: line range outside the PR's diff.
.hierarchy-boundary-heading { | ||
font-size: 2.5rem; | ||
font-weight: 700; | ||
font-family: 'Roboto Condensed', sans-serif; | ||
} | ||
|
||
.hierarchy-boundary-sub-heading { | ||
width: 20rem; | ||
margin-top: 0.6rem; | ||
font-weight: 600; | ||
font-family: 'Roboto', sans-serif; | ||
} | ||
|
||
.hierarchy-boundary-sub-heading2 { | ||
font-size: 1.2rem; | ||
font-weight: 600; | ||
font-family: 'Roboto', sans-serif; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Use SCSS variables for typography values.
Consider extracting font sizes, weights, and families into SCSS variables for better maintainability and consistency.
+// At the top of the file or in a separate variables file
+$heading-font-size: 2.5rem;
+$subheading-font-size: 1.2rem;
+$primary-font-family: 'Roboto', sans-serif;
+$condensed-font-family: 'Roboto Condensed', sans-serif;
+$font-weight-bold: 700;
+$font-weight-semibold: 600;
.hierarchy-boundary-heading {
- font-size: 2.5rem;
- font-weight: 700;
- font-family: 'Roboto Condensed', sans-serif;
+ font-size: $heading-font-size;
+ font-weight: $font-weight-bold;
+ font-family: $condensed-font-family;
}
Committable suggestion skipped: line range outside the PR's diff.
@@ -90,7 +92,7 @@ | |||
|
|||
.digit-popup-wrapper { | |||
&.popUpClass { | |||
width: 45rem; | |||
width: 50rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codebase verification
Add media queries for responsive popup width
The codebase shows established patterns of responsive design with media queries for different screen sizes (mobile: 30rem, tablet: 30.063rem to 47.938rem). The popup's fixed width of 50rem will cause horizontal scrolling on smaller screens. Add the following media queries:
- For screens below 48rem (768px), set width to 90vw
- For screens below 30rem (480px), set width to 95vw
Example locations that need updates:
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss
: Add media queries to.digit-popup-wrapper.popUpClass
🔗 Analysis chain
Verify popup responsiveness on smaller screens.
The increased popup width of 50rem might cause issues on smaller screens. Consider adding a media query for responsive behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing media query implementations in SCSS files
rg -g "*.scss" "(@media|@include)" --type scss
Length of output: 77
Script:
#!/bin/bash
# Search for any existing media query implementations in CSS/SCSS files using a more generic approach
rg "(@media|@include)" health/micro-ui/web/micro-ui-internals/packages/css/src/
Length of output: 15878
Script:
#!/bin/bash
# Let's check the specific campaign.scss file for any existing media queries and the context around the width setting
rg -A 5 -B 5 "width: 50rem" health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss
Length of output: 302
.employee { | ||
.digit-employeeSidebar { | ||
.sidebar { | ||
z-index: 999; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Standardize z-index values to prevent stacking order issues
Multiple elements have high z-index values (e.g., 999, 9999, 10000), which can cause stacking context conflicts and make the UI difficult to manage. Consider defining a z-index scale using variables or a centralized management system to ensure consistency and avoid unintended overlapping of elements.
Also applies to: 194-194, 197-197, 200-200, 203-203
@keyframes slideInFromBottom { | ||
from { | ||
bottom: -100%; | ||
} | ||
to { | ||
bottom: 4.5rem; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid redefining @keyframes
with the same name
The @keyframes slideInFromBottom
is already defined earlier in the file. Redefining it can cause unexpected behavior in animations. Please rename the new keyframes if different animations are intended, or consolidate them into a single definition if they are meant to be the same.
animation: slideInFromBottom 0.5s ease forwards; | ||
} | ||
.horizontal-nav { | ||
background-color: transparent !important; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Avoid using !important
in .horizontal-nav
styles
Using !important
can lead to specificity wars and maintenance difficulties. Adjust the CSS specificity or cascade to apply the background color without relying on !important
.
} | ||
} | ||
.digit-card-label { | ||
width: 30%; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Ensure responsive design for .digit-card-label
width
Setting a fixed width of 30% may not adapt well on all screen sizes. Consider using relative units or responsive design techniques like flexbox or grid layouts to ensure the label adjusts appropriately on different devices.
color: theme(digitv2.lightTheme.primary); | ||
} | ||
.horizontal-nav .sidebar-list.active { | ||
border-bottom: 3px solid #c84c0e; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Replace hard-coded color value with a theme variable
The border-bottom color #c84c0e
in .horizontal-nav .sidebar-list.active
is hard-coded. To maintain consistency and ease of theming, replace it with a theme variable like theme(digitv2.lightTheme.primary)
.
Apply this diff to implement the change:
.horizontal-nav .sidebar-list.active {
- border-bottom: 3px solid #c84c0e;
+ border-bottom: 3px solid theme(digitv2.lightTheme.primary);
}
📝 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.
border-bottom: 3px solid #c84c0e; | |
border-bottom: 3px solid theme(digitv2.lightTheme.primary); |
* added list of assignee for pop and plan inbox * updated comments * updated assignee for plan inbox --------- Co-authored-by: rachana-egov <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryRelationCreate.js
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryRelationCreate.js (1)
Pattern **/*.js
: check
🪛 Biome (1.9.4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryRelationCreate.js
[error] 308-308: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the try/catch clause.
(lint/complexity/noUselessCatch)
[error] 454-454: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 460-460: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 462-462: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 467-467: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 479-479: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 503-503: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 509-509: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 525-525: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 584-584: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 586-586: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 607-607: Enforce to have the onClick mouse event with the onKeyUp, the onKeyDown, or the onKeyPress keyboard event.
Actions triggered using mouse events should have corresponding keyboard events to account for keyboard-only navigation.
(lint/a11y/useKeyWithClickEvents)
[error] 613-613: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 23-23: This let declares a variable that is only assigned once.
'locale' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 27-27: This let declares a variable that is only assigned once.
'receivedData' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 73-73: This let declares a variable that is only assigned once.
'flag' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 235-235: This let declares a variable that is only assigned once.
'errorString' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 489-489: 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] 534-542: 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] 543-560: 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] 589-589: 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] 634-642: 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] 643-662: 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 (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/pages/employee/BoundaryRelationCreate.js (1)
35-35
:
Define the module
variable before usage
The module
variable is used but not defined, which will cause a ReferenceError
. Ensure that module
is properly defined before using it in useUpsertLocalisation
.
Apply this diff to define the module
variable:
+ const module = `hcm-boundary-${hierarchyType.toLowerCase()}`;
const { mutateAsync: localisationMutateAsync } = Digit.Hooks.campaign.useUpsertLocalisation(tenantId, module, locale);
Likely invalid or redundant comment.
onClick={() => { | ||
const checkValid = newBoundaryData?.every((obj) => obj?.boundaryType); | ||
if (checkValid) { | ||
setShowFinalPopup(true); | ||
} else if (newBoundaryData?.some((obj) => obj?.boundaryType)) { | ||
setShowToast({ label: t("CMN_FILLORDELETE_CREATED_HIERARCHY"), isError: "error" }); | ||
} else { | ||
setShowToast({ label: t("CMN_ATLEAST_ONE_HIERARCHY"), isError: "error" }); | ||
} | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Extract inline function for better readability
The onClick
handler contains complex logic implemented inline, which can affect readability and maintainability. Consider extracting this logic into a separate function.
Apply this refactor to improve code clarity:
- onClick={() => {
- const checkValid = newBoundaryData?.every((obj) => obj?.boundaryType);
- if (checkValid) {
- setShowFinalPopup(true);
- } else if (newBoundaryData?.some((obj) => obj?.boundaryType)) {
- setShowToast({ label: t("CMN_FILLORDELETE_CREATED_HIERARCHY"), isError: "error" });
- } else {
- setShowToast({ label: t("CMN_ATLEAST_ONE_HIERARCHY"), isError: "error" });
- }
- }}
+ onClick={handleCreateClick}
Define the handleCreateClick
function outside the JSX:
const handleCreateClick = () => {
const checkValid = newBoundaryData?.every((obj) => obj?.boundaryType);
if (checkValid) {
setShowFinalPopup(true);
} else if (newBoundaryData?.some((obj) => obj?.boundaryType)) {
setShowToast({ label: t("CMN_FILLORDELETE_CREATED_HIERARCHY"), isError: "error" });
} else {
setShowToast({ label: t("CMN_ATLEAST_ONE_HIERARCHY"), isError: "error" });
}
};
const res11 = await downloadExcelTemplate(); | ||
|
||
// Only proceed with data creation if template generation was successful | ||
if(newHierarchy === false) | ||
{ | ||
const res22 = await callCreateDataApi(res11); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧹 Nitpick (assertive)
Improve variable naming for clarity
Variables like res11
and res22
are not descriptive and can make the code harder to understand. Consider renaming them to more meaningful names that reflect their purpose.
Apply this diff to enhance code readability:
- const res11 = await downloadExcelTemplate();
+ const fileStoreId = await downloadExcelTemplate();
- if(newHierarchy === false)
- {
- const res22 = await callCreateDataApi(res11);
- }
+ if (newHierarchy === false) {
+ await callCreateDataApi(fileStoreId);
+ }
📝 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 res11 = await downloadExcelTemplate(); | |
// Only proceed with data creation if template generation was successful | |
if(newHierarchy === false) | |
{ | |
const res22 = await callCreateDataApi(res11); | |
} | |
const fileStoreId = await downloadExcelTemplate(); | |
// Only proceed with data creation if template generation was successful | |
if (newHierarchy === false) { | |
await callCreateDataApi(fileStoreId); | |
} |
addParents(); | ||
createNewHierarchy(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure asynchronous operations are properly handled
In the onConfirmClick
function, you are calling addParents()
and createNewHierarchy()
without awaiting them. If these functions perform asynchronous operations, consider handling them appropriately to ensure the sequence of execution is maintained.
Apply this diff if the functions are asynchronous:
-const onConfirmClick = () => {
+const onConfirmClick = async () => {
addParents();
- createNewHierarchy();
+ await createNewHierarchy();
}
This ensures that createNewHierarchy()
completes before proceeding further.
Committable suggestion skipped: line range outside the PR's diff.
item.boundaryType = item.boundaryType.trim(); | ||
if (idx === 0) { | ||
if (newHierarchy) item.parentBoundaryType = null; | ||
else { | ||
if (boundaryData.length === 0) item.parentBoundaryType = null; | ||
else item.parentBoundaryType = boundaryData[boundaryData.length - 1].boundaryType.trim(); | ||
|
||
} | ||
} | ||
if (idx > 0) { | ||
item.parentBoundaryType = prevItems[idx - 1].boundaryType.trim(); | ||
} | ||
return item; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid direct state mutation inside setState
In the addParents
function, you are mutating state directly within the setNewBoundaryData
updater function, which can lead to unexpected behavior. Instead, create new objects to maintain immutability.
Apply this diff to prevent direct state mutation:
const addParents = () => {
setNewBoundaryData((prevItems) => {
return prevItems.map((item, idx) => {
- item.boundaryType = item.boundaryType.trim();
+ const newItem = { ...item, boundaryType: item.boundaryType.trim() };
if (idx === 0) {
if (newHierarchy)
- item.parentBoundaryType = null;
+ newItem.parentBoundaryType = null;
else {
if (boundaryData.length === 0)
- item.parentBoundaryType = null;
+ newItem.parentBoundaryType = null;
else
- item.parentBoundaryType = boundaryData[boundaryData.length - 1].boundaryType.trim();
+ newItem.parentBoundaryType = boundaryData[boundaryData.length - 1].boundaryType.trim();
}
}
if (idx > 0) {
- item.parentBoundaryType = prevItems[idx - 1].boundaryType.trim();
+ newItem.parentBoundaryType = prevItems[idx - 1].boundaryType.trim();
}
- return item;
+ return newItem;
});
});
}
📝 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.
item.boundaryType = item.boundaryType.trim(); | |
if (idx === 0) { | |
if (newHierarchy) item.parentBoundaryType = null; | |
else { | |
if (boundaryData.length === 0) item.parentBoundaryType = null; | |
else item.parentBoundaryType = boundaryData[boundaryData.length - 1].boundaryType.trim(); | |
} | |
} | |
if (idx > 0) { | |
item.parentBoundaryType = prevItems[idx - 1].boundaryType.trim(); | |
} | |
return item; | |
}); | |
const newItem = { ...item, boundaryType: item.boundaryType.trim() }; | |
if (idx === 0) { | |
if (newHierarchy) newItem.parentBoundaryType = null; | |
else { | |
if (boundaryData.length === 0) newItem.parentBoundaryType = null; | |
else newItem.parentBoundaryType = boundaryData[boundaryData.length - 1].boundaryType.trim(); | |
} | |
} | |
if (idx > 0) { | |
newItem.parentBoundaryType = prevItems[idx - 1].boundaryType.trim(); | |
} | |
return newItem; | |
}); |
…heck, (#1884) * added serving facility * form composer action bar fix * plan inbox assignee fix * formula and assumption refresh issue * action bar hidden fixes, session fixes * fixes * fix * action bar fix, formula next back issue * assumption and formula one mdms check and refetch blank label check * remove status log column * fix * adhoc fix * fix * fix * del session --------- Co-authored-by: NabeelAyubee <[email protected]>
Choose the appropriate template for your PR:
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Style