Boundary screen #1451
Boundary screen #1451
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes involve an update to the CSS stylesheet link within the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Outside diff range comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (3)
Line range hint
30-37: Reconsider the useEffect hook implementationThe current useEffect hook implementation with an execution counter might lead to unexpected behavior:
useEffect(() => { if (executionCount < 5) { onSelect(customProps.name, { distributionStrat,disease,campaignType }); setExecutionCount((prevCount) => prevCount + 1); } });This effect runs on every render (no dependency array) and updates the state, which triggers another render. This could lead to unnecessary re-renders and potential performance issues. Consider:
- Adding a dependency array to control when this effect should run.
- If the intention is to run this effect only on mount, use a ref instead of state to track the execution count.
- If this effect is meant to sync data, consider using the other useEffect that already exists for this purpose.
Here's a potential refactor:
useEffect(() => { onSelect(customProps.name, { distributionStrat, disease, campaignType }); }, [distributionStrat, disease, campaignType, customProps.name, onSelect]);This will ensure the effect only runs when the relevant data changes.
Line range hint
20-21: Clarify the isFreezed state initializationThe isFreezed state is initialized based on campaignId and microplanId:
const [isFreezed,setIsFreezed] = useState(campaignId && microplanId ? true : false);However, these variables are not defined within the component scope. They seem to come from query parameters, but this isn't immediately clear. Consider:
- Destructuring these values from queryParams at the top of the component for clarity.
- Adding a comment explaining the purpose of the isFreezed state and when it should be true.
Here's a suggested refactor:
const { campaignId, microplanId } = queryParams; // isFreezed is true when editing an existing campaign const [isFreezed, setIsFreezed] = useState(Boolean(campaignId && microplanId));
Line range hint
1-141: Consider performance optimizations and code structure improvementsWhile the overall structure of the component is sound, there are a few areas where it could be optimized:
Memoization: Consider using useMemo for expensive computations or React.memo for child components that don't need frequent re-renders.
Callback optimization: The onSelect function passed as a prop could be wrapped in useCallback to prevent unnecessary re-renders of child components.
Custom hook: The data fetching logic using Digit.Hooks.useCustomMDMS could potentially be moved to a custom hook for better separation of concerns.
Error handling: There's no visible error handling for the data fetching. Consider adding error states and displaying appropriate messages to the user.
Accessibility: Ensure all interactive elements are keyboard accessible and have appropriate aria labels.
Here's an example of how you might optimize the onSelect callback:
const memoizedOnSelect = useCallback((name, data) => { onSelect(name, data); }, [onSelect]); // Then use memoizedOnSelect instead of onSelect in your useEffectConsider breaking down this component into smaller, more focused components. For example, you could create separate components for each dropdown (DiseaseDropdown, CampaignTypeDropdown, etc.). This would improve readability and make the component easier to maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (8)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/subBoundaryView.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/Sample.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (2 hunks)
- health/micro-ui/web/public/index.html (1 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/subBoundaryView.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/Sample.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
[error] 463-471: 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] 488-496: 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] 329-329: This let declares a variable that is only assigned once.
'parents' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 330-330: This let declares a variable that is only assigned once.
'parent_group' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 381-381: This let declares a variable that is only assigned once.
'bH' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 383-383: This let declares a variable that is only assigned once.
'dic' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 386-386: This let declares a variable that is only assigned once.
'bType' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 387-387: This let declares a variable that is only assigned once.
'parent' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 452-452: 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] 476-476: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js
[error] 154-154: 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 (12)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/subBoundaryView.js (3)
1-4: LGTM: Import statements are appropriate.The import statements are correctly structured and import the necessary components for the functionality of this file. The use of the @egovernments/digit-ui-components package suggests adherence to a consistent design system across the application.
46-46: LGTM: Component export is correct.The default export of the SubBoundaryView component is properly implemented, following common React practices.
16-32: 🛠️ Refactor suggestionImprove Chip component implementation and consider error handling.
The Chip rendering logic is generally good, but there are a few areas for improvement:
- The empty onClick and onTagClick handlers suggest incomplete functionality. Either implement these handlers or remove them if they're not needed.
- Consider moving the inline styles to a separate CSS file or using styled-components for better maintainability.
- The error prop is empty. If error handling is not implemented, consider removing this prop.
Here's a suggested refactor:
{arr.map((el, ind) => ( - <div key={ind} className="digit-tag-container" style={{ minWidth: "1rem" }} > + <div key={ind} className="digit-tag-container"> <Chip - className="" - error="" extraStyles={{ color: 'red', display: "inline-block", }} - onClick={() => { }} - onTagClick={() => { }} text={el} /> </div> ))}Also, consider implementing the onClick and onTagClick handlers if they're needed for functionality:
const handleChipClick = (el) => { // Implement chip click logic }; const handleTagClick = (el) => { // Implement tag click logic }; // Then in the Chip component: onClick={() => handleChipClick(el)} onTagClick={() => handleTagClick(el)}To ensure there are no unused props or methods in the Chip component, let's check its implementation:
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js (3)
18-18: LGTM: New import for CampaignBoundary component.The import statement for the new
CampaignBoundarycomponent is correctly added and aligns with the PR objective of implementing a Campaign Boundary screen.
65-65: LGTM: CampaignBoundary component added to registry.The
CampaignBoundarycomponent has been correctly added to thecomponentsToRegisterobject, which aligns with the PR objective of implementing a Campaign Boundary screen.
Line range hint
69-74: Verify the integration of the new CampaignBoundary component.The
initMicroplanComponentsfunction remains unchanged, which is correct as it will automatically register the newCampaignBoundarycomponent along with others. However, it's important to ensure that the new component is properly utilized where needed in the application.To verify the usage of the new component, you can run the following script:
This will help confirm that the new component is being used appropriately in the application.
✅ Verification successful
Verified: Proper Integration of CampaignBoundary Component
The
CampaignBoundarycomponent is correctly registered inModule.jsand appropriately utilized in the application:
- Imported and registered in
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/Module.js.- Used within
<PrivateRoute>inhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js.- Implemented as a component in
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of CampaignBoundary component in the codebase # Test: Look for CampaignBoundary usage rg --type js 'CampaignBoundary'Length of output: 1058
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (2)
9-9: LGTM: Import statement for CampaignBoundary is correct.The import statement for the CampaignBoundary component is properly formatted and uses the correct relative path.
154-155: Verify integration with summary screen.The PR objectives mention that this Campaign Boundary screen is intended for the summary screen. However, the current implementation adds it as a separate route (
${path}/cb).Could you please clarify how this new route integrates with or relates to the summary screen mentioned in the PR objectives? Is there additional code or are there plans to link this new component to the existing SummaryScreen component?
To help verify this, you can run the following script to check for any connections between the CampaignBoundary and SummaryScreen components:
This will help us understand if and how these components are connected, ensuring alignment with the PR objectives.
✅ Verification successful
Integration with Summary Screen Verified.
The
CampaignBoundarycomponent is successfully integrated into theSummaryScreenas intended.
- File:
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for connections between CampaignBoundary and SummaryScreen # Search for imports or uses of CampaignBoundary in SummaryScreen echo "Searching for CampaignBoundary in SummaryScreen:" rg --type js "CampaignBoundary" -C 5 $(fd -e js "SummaryScreen") # Search for imports or uses of SummaryScreen in CampaignBoundary echo "Searching for SummaryScreen in CampaignBoundary:" rg --type js "SummaryScreen" -C 5 $(fd -e js "CampaignBoundary")Length of output: 871
🧰 Tools
🪛 Biome
[error] 154-154: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js (3)
Line range hint
1-574: Overall, the changes look good and integrate well with the existing component.The addition of the Campaign Boundary card and its corresponding navigation item have been implemented correctly. The changes follow the existing patterns in the code and should work as expected. Make sure to address the verification points mentioned in the previous comments to ensure full functionality and maintainability of the component.
470-474: LGTM! Horizontal navigation updated correctly for Campaign Boundary.The new navigation item for the Campaign Boundary card has been added correctly to the
horizontalNav.configNavItemsarray. The properties are consistent with other navigation items.Please ensure that the translation key "CAMPAIGN_BOUNDARY" exists in the translation files. Run the following script to check:
#!/bin/bash # Description: Verify the existence of the "CAMPAIGN_BOUNDARY" translation key # Test: Search for the "CAMPAIGN_BOUNDARY" translation key in all JSON files echo "Searching for 'CAMPAIGN_BOUNDARY' translation key:" rg --type json '"CAMPAIGN_BOUNDARY"' health/micro-ui/web/micro-ui-internals/packages/modules/microplan/public/localization/If the key is not found, make sure to add it to the appropriate translation files.
99-116: LGTM! New card for Campaign Boundary added successfully.The new card for Campaign Boundary has been added correctly, following the structure of other cards in the array. However, please verify if the CampaignBoundary component requires any props. If it does, make sure to add them to the empty props object.
To ensure the CampaignBoundary component is properly implemented and imported, run the following script:
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1)
358-362: Ensure state initialization handles undefined valuesThe
boundaryHierarchystate could beundefined, leading to potential errors when accessingboundaryHierarchy.length. Provide a default value or add a check to prevent errors.Please verify that
state["boundaryHierarchy"]is defined. If it's possible for it to beundefined, consider setting a default value:- 358: const [boundaryHierarchy, setData] = useState(state["boundaryHierarchy"]); + 358: const [boundaryHierarchy, setData] = useState(state["boundaryHierarchy"] || []);
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/subBoundaryView.js
Outdated
Show resolved
Hide resolved
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/subBoundaryView.js
Outdated
Show resolved
Hide resolved
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js
Outdated
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Outdated
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/Sample.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (1)
Line range hint
1-160: Overall review: Implementation seems incomplete.The changes in this file partially align with the PR objective of implementing a Campaign Boundary screen. While the
CampaignBoundarycomponent is imported, the corresponding route is missing. To complete the implementation:
- Add the
PrivateRoutefor the CampaignBoundary component as mentioned in the AI summary.- Ensure that the new route integrates well with the existing routing structure and data flow.
- Verify that the
CampaignBoundarycomponent is properly implemented and can accept data as mentioned in the PR description.Once these steps are completed, the changes will fully meet the PR objectives.
🧰 Tools
🪛 Biome
[error] 153-153: 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)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (5)
- health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/subBoundaryView.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js (2 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/subBoundaryView.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (1)
Pattern
**/*.js: check
📓 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-06-14T14:10:38.086Z Learning: Classes related to interactive elements in the microplan preview section are mostly passed to Higher Order Components (HOCs), and ARIA attributes for non-HOC elements will be managed directly by adding them where necessary.
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
[error] 164-167: 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] 184-187: 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] 34-34: This let declares a variable that is only assigned once.
'parents' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 35-35: This let declares a variable that is only assigned once.
'parent_group' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 86-86: This let declares a variable that is only assigned once.
'bH' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 88-88: This let declares a variable that is only assigned once.
'dic' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 91-91: This let declares a variable that is only assigned once.
'bType' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 92-92: This let declares a variable that is only assigned once.
'parent' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🔇 Additional comments (10)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/subBoundaryView.js (4)
1-4: LGTM: Import statements are appropriate.The import statements are concise and import only the necessary components. Good job on avoiding unused imports.
6-6: LGTM: Component declaration and props destructuring are well-implemented.The functional component is correctly declared using arrow function syntax, and props are properly destructured. This approach enhances readability and follows React best practices.
9-37: LGTM: Conditional rendering logic is well-implemented.The component correctly checks for the existence and non-emptiness of the
arrprop before rendering the Card component. This prevents unnecessary rendering when there's no data to display. The use of a ternary operator for this conditional rendering is concise and appropriate.
45-45: LGTM: Component export is correct.The component is properly exported as the default export, which is a common and acceptable practice in React.
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss (1)
100-113: Summary of changes and alignment with PR objectivesThe new classes
.subBoundarycomp-containerand.view-moreappear to be related to the implementation of the Campaign Boundary screen mentioned in the PR objectives. These styles will likely improve the layout and interactivity of the summary screen.The changes are in line with the PR objectives. Once the syntax errors are corrected, these styles should enhance the user interface for displaying campaign boundary information. Consider adding comments to explain the purpose of these classes for better maintainability.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/index.js (1)
9-9: LGTM: New component import added correctly.The import statement for the
CampaignBoundarycomponent is correctly added and aligns with the PR objective of implementing a Campaign Boundary screen.health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js (1)
Line range hint
1-524: Overall, the changes successfully implement the Campaign Boundary screen.The additions to the
SummaryScreencomponent effectively integrate the new Campaign Boundary section into the existing summary screen structure. The changes align well with the PR objective of implementing a Campaign Boundary screen for the summary screen.A few minor suggestions have been made to further improve code quality and organization. Great job on maintaining consistency with the existing code structure!
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (3)
164-167: Existing comment about adding keyboard event handlers is still applicableThe
<div>element at lines 164-167 with anonClickhandler should also handle keyboard events to enhance accessibility for users navigating via keyboard. Additionally, making the element focusable by addingtabIndex="0"can improve accessibility. The previous review comment on this issue is still valid.🧰 Tools
🪛 Biome
[error] 164-167: 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)
184-187: Existing comment about adding keyboard event handlers is still applicableThe
<div>element at lines 184-187 with anonClickhandler should also handle keyboard events to enhance accessibility for users navigating via keyboard. Additionally, making the element focusable by addingtabIndex="0"can improve accessibility. The previous review comment on this issue is still valid.🧰 Tools
🪛 Biome
[error] 184-187: 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)
175-175: Existing comment about incorrect use offilterfunction parameters is still applicableThe
filterfunction at line 175 is not using its parameters, which might lead to unintended results. The callback should accept parameters and use them appropriately. The past review comment on this issue is still valid and applicable.
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/subBoundaryView.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss
Outdated
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss
Outdated
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js
Show resolved
Hide resolved
...cro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SummaryScreen.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
Global configuration variable
REACT_APP_GLOBALis not setThe
REACT_APP_GLOBALenvironment variable is not defined in any environment files (.env*). Ensure thatREACT_APP_GLOBALis properly set in all relevant environment configurations to enable the loading of the global configuration.🔗 Analysis chain
Line range hint
29-30: Verify global configuration loadingThe global configuration is loaded from an environment variable (
%REACT_APP_GLOBAL%). This approach is generally good for flexibility, but it's important to ensure that this variable is properly set in all environments.To verify the global configuration setup, please run the following checks:
Ensure that the
REACT_APP_GLOBALvariable is properly set in all relevant environment files (.env, .env.production, etc.) and that there are no hardcoded references toglobalConfigs.jsin the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify global configuration setup # Test: Check if REACT_APP_GLOBAL is defined in environment files rg "REACT_APP_GLOBAL=" .env* # Test: Search for any hardcoded references to globalConfigs.js rg "globalConfigs.js"Length of output: 708
📜 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.jsonis excluded by!**/*.json
📒 Files selected for processing (4)
- health/micro-ui/web/micro-ui-internals/example/public/index.html (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1)
Pattern
**/*.js: checkhealth/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (1)
Pattern
**/*.js: check
📓 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-06-14T14:10:38.086Z Learning: Classes related to interactive elements in the microplan preview section are mostly passed to Higher Order Components (HOCs), and ARIA attributes for non-HOC elements will be managed directly by adding them where necessary.
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
[error] 164-167: 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] 184-187: 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] 34-34: This let declares a variable that is only assigned once.
'parents' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 35-35: This let declares a variable that is only assigned once.
'parent_group' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 86-86: This let declares a variable that is only assigned once.
'bH' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 88-88: This let declares a variable that is only assigned once.
'dic' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 91-91: This let declares a variable that is only assigned once.
'bType' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 92-92: This let declares a variable that is only assigned once.
'parent' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🔇 Additional comments (6)
health/micro-ui/web/micro-ui-internals/example/public/index.html (2)
Line range hint
13-28: Consider stylesheet organization and version consistencyThe file contains multiple stylesheet links with different versions and purposes. Some considerations:
- There are commented-out stylesheet links. Consider removing them if they're no longer needed.
- Multiple versions of
@egovernments/digit-ui-cssare being loaded (1.8.2-beta.11, 1.8.0-alpha.6, 1.0.73-campaign, 1.0.50-microplan). This could lead to style conflicts or unnecessary bloat.- Some stylesheets are using development versions (alpha, beta). For production, it's generally better to use stable versions.
Consider consolidating and organizing the stylesheets:
- Remove commented-out links.
- Use a single, stable version of
@egovernments/digit-ui-cssif possible.- Ensure all stylesheets are using production-ready versions.
Example refactor (adjust versions as needed):
- <!-- <link - rel="stylesheet" - href="https://unpkg.com/@egovernments/digit-ui-css@1.8.0-alpha.6/dist/index.css" - /> --> - <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css@1.8.2-beta.11/dist/index.css" /> - <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css@1.8.0-alpha.6/dist/index.css" /> - - <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-components-css@0.0.2-beta.34/dist/index.css" /> - <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css@1.0.73-campaign/dist/index.css" /> - - <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css@1.0.50-microplan/dist/index.css" /> - - <!-- added below css for hcm-workbench module inclusion--> - <!-- <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css@1.5.41/dist/index.css" /> --> - - <!-- <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css/dist/index.css"/> --> + <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-css@1.0.73-campaign/dist/index.css" /> + <link rel="stylesheet" href="https://unpkg.com/@egovernments/digit-ui-components-css@1.0.0/dist/index.css" />To ensure all necessary styles are included after consolidation, please run the following verification steps:
21-21: Stylesheet version updatedThe stylesheet version for
@egovernments/digit-ui-csshas been updated from 1.0.72-campaign to 1.0.73-campaign. This change likely includes new styles or fixes for the Campaign Boundary screen.To ensure this update doesn't introduce any unexpected visual changes or regressions, please run the following verification steps:
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss (3)
100-104: LGTM after applying the suggested fixes.The
.subBoundarycomp-containerclass looks good conceptually. Please apply the fixes suggested in the previous comment to correct the syntax errors. Once those changes are made, this addition will be approved.
106-111: LGTM after applying the suggested fixes.The
.view-moreclass looks good conceptually. Please apply the fixes suggested in the previous comment to correct the syntax errors and improve formatting. Once those changes are made, this addition will be approved.
99-111: Overall, the new styles enhance the UI components.The additions of
.subBoundarycomp-containerand.view-moreclasses contribute to the styling of the Campaign Boundary screen. Once the syntax errors are fixed as suggested in the previous comments, these styles will improve the layout and interactivity of the components.Make sure to test these styles in the context of the Campaign Boundary screen to ensure they achieve the desired visual effect.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (1)
68-69: LGTM!No issues found in the code changes.
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1)
Pattern
**/*.js: check
📓 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-06-14T14:10:38.086Z Learning: Classes related to interactive elements in the microplan preview section are mostly passed to Higher Order Components (HOCs), and ARIA attributes for non-HOC elements will be managed directly by adding them where necessary.
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
[error] 382-385: 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] 402-405: 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] 252-252: This let declares a variable that is only assigned once.
'parents' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 253-253: This let declares a variable that is only assigned once.
'parent_group' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 304-304: This let declares a variable that is only assigned once.
'bH' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 306-306: This let declares a variable that is only assigned once.
'dic' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 309-309: This let declares a variable that is only assigned once.
'bType' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 310-310: This let declares a variable that is only assigned once.
'parent' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
🔇 Additional comments (4)
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss (3)
100-104: LGTM! Syntax errors fixed.The
.subBoundarycomp-containerclass has been correctly implemented. The previous syntax errors with quoted values have been addressed, and the properties are now properly set for a flex container. This implementation aligns well with the PR objectives for the Campaign Boundary screen.
106-111: LGTM! Syntax errors and formatting issues resolved.The
.view-moreclass has been correctly implemented. The previous syntax errors with quoted values and unnecessary spacing have been addressed. The styles are now properly set for an interactive "view more" element, which aligns well with the PR objectives for the Campaign Boundary screen summary.
99-111: Overall assessment: Changes look good and align with PR objectives.The new classes
.subBoundarycomp-containerand.view-morehave been implemented correctly, addressing previous syntax and formatting issues. These styles contribute to the layout and interactivity of the Campaign Boundary screen, which is the main objective of this PR. The changes are concise, focused, and free of any apparent issues.health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1)
14-27: LGTM:handleViewMorefunction correctly updates stateThe
handleViewMorefunction efficiently toggles theboundaryStatusstate at the specified index. The use ofupdatedBoundaryStatusensures immutability, which is a good practice when updating state in React.
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Outdated
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js (1)
Pattern
**/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
[error] 166-169: 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] 186-189: 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] 36-36: This let declares a variable that is only assigned once.
'parents' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 37-37: This let declares a variable that is only assigned once.
'parent_group' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 88-88: This let declares a variable that is only assigned once.
'bH' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 90-90: This let declares a variable that is only assigned once.
'dic' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 93-93: This let declares a variable that is only assigned once.
'bType' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
[error] 94-94: This let declares a variable that is only assigned once.
'parent' is never reassigned.
Safe fix: Use const instead.
(lint/style/useConst)
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignBoundary.js
Show resolved
Hide resolved
Campaign Boundary screen for summary screen, ready to take data.
Summary by CodeRabbit
Release Notes
New Features
CampaignBoundarycomponent for enhanced campaign boundary management.SubBoundaryViewcomponent to display boundary details.selectedDatastructure for geographical entities in the Sample page.Bug Fixes
CampaignDetailscomponent for improved user experience.Style
Chores