Info icon revert changes #1801
Info icon revert changes #1801
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to several files primarily focused on modifying CSS stylesheet links and enhancing component styles and functionality. The CSS version has been incremented from Changes
Possibly related PRs
Suggested reviewers
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: 4
🧹 Outside diff range comments (4)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
Line range hint
1-33: Consider performance optimizations for CSS loading.The file loads multiple CSS files from unpkg.com CDN. Consider the following optimizations:
- Combine CSS files to reduce HTTP requests
- Use preload/prefetch hints for critical CSS
- Consider self-hosting these files for better control over caching and availability
Example implementation:
+ <link rel="preload" as="style" href="https://unpkg.com/@egovernments/[email protected]/dist/index.css" />Would you like assistance in implementing these optimizations or creating a GitHub issue to track this improvement?
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (1)
Line range hint
1-576: Consider component decomposition and state management improvementsThe component handles multiple complex responsibilities which could benefit from better separation of concerns:
- Formula CRUD operations
- State management for formulas and deletions
- Multiple popup dialogs
- Complex UI rendering
Consider these improvements:
- Extract formula management logic into a custom hook (e.g.,
useFormulaManagement)- Split the component into smaller, focused components:
- FormulaList
- FormulaItem
- DeleteConfirmationDialog
- AddFormulaDialog
- Replace ref-based deletion tracking with proper state management
- Consider using a reducer for complex state updates
Would you like assistance in implementing any of these architectural improvements?
🧰 Tools
🪛 Biome
[error] 307-307: 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] 313-313: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (2)
Line range hint
1-2624: Consider improving SCSS structure and maintainability.The SCSS file could benefit from the following improvements:
- Extract repeated color values and dimensions into variables
- Reduce nesting depth to improve maintainability
- Consolidate media queries for better responsive design management
- Split into smaller, focused files (e.g.,
_upload.scss,_hypothesis.scss, etc.)Example of extracting variables:
- border: 0.063rem solid rgba(214, 213, 212, 1); + $border-width: 0.063rem; + $border-color-light: rgba(214, 213, 212, 1); + border: $border-width solid $border-color-light;
Line range hint
1-2624: Enhance accessibility through improved styles.Consider the following accessibility improvements:
- Ensure color contrast ratios meet WCAG 2.1 Level AA standards (especially for text colors)
- Enhance focus states for better keyboard navigation
- Add styles for ARIA states (e.g.,
aria-selected,aria-expanded)Example for focus states:
+ .interactive-element:focus-visible { + outline: 0.188rem solid $primary-theme-color; + outline-offset: 0.125rem; + }
📜 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 (5)
health/micro-ui/web/micro-ui-internals/example/public/index.html(1 hunks)health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js(1 hunks)health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js(1 hunks)health/micro-ui/web/public/index.html(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1)
Pattern **/*.js: check
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.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.
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
[error] 313-313: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
[error] 160-160: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
🔇 Additional comments (2)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
15-15: Verify the latest stable version of digit-ui-health-css.
While updating to version 0.1.31 is consistent with the PR objectives, let's verify if this is the latest stable version and check for any security advisories.
✅ Verification successful
Let me gather more information about the package versions in the repository to better understand the versioning pattern.
Version 0.1.31 is the correct version to use
The version update is verified to be correct based on the following findings:
- The package.json in the repository defines version 0.1.31 as the current version
- The same version is consistently used in multiple HTML files in the codebase
- While npm shows 0.1.0 as latest, this appears to be a registry sync issue as the actual package in the codebase is at 0.1.31
- No security vulnerabilities were found for this package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the latest version and security status of digit-ui-health-css package
# Check NPM for latest versions of the package
curl -s https://registry.npmjs.org/@egovernments/digit-ui-health-css | jq '.["dist-tags"].latest'
# Check for any security advisories
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: NPM, package: "@egovernments/digit-ui-health-css") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'
Length of output: 474
Script:
#!/bin/bash
# Search for other occurrences of digit-ui-health-css version references
rg -i "digit-ui-health-css@" -A 1 -B 1
# Check package.json files for any references to this package
fd "package.json" --exec grep -l "digit-ui-health-css" {} \; --exec cat {}
Length of output: 2793
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (1)
2591-2591: LGTM: Icon spacing adjustment looks good.
The increased left margin (0.3rem) for the icon wrapper provides better visual spacing between the assumption label text and its associated icon.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
Outdated
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
Outdated
Show resolved
Hide resolved
...-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (3)
Line range hint
191-193: Remove redundant isDisabled logicThe
isAddNewDisabledcalculation is being overridden with a hardcodedfalse. Either remove the unused logic or use the calculated value.Apply one of these changes:
- <Button - icon="Add" - label={category === "CAMPAIGN_VEHICLES" ? t(`ADD_ASSUMPTION_${category}`) : t("ADD_ASSUMPTION")} - onClick={() => setAssumptionsPopUp(true)} - variation="secondary" - // isDisabled={isAddNewDisabled} - isDisabled={false} - />Option 1: Remove unused logic:
- const isAddNewDisabled = - !deletedAssumptionCategories.current[category] || - deletedAssumptionCategories.current[category].length === 0 || - deletedAssumptionCategories.current[category].every((item) => !deletedAssumptions.includes(item)); // In Button component <Button icon="Add" label={category === "CAMPAIGN_VEHICLES" ? t(`ADD_ASSUMPTION_${category}`) : t("ADD_ASSUMPTION")} onClick={() => setAssumptionsPopUp(true)} variation="secondary" isDisabled={false} />Option 2: Use calculated value:
<Button icon="Add" label={category === "CAMPAIGN_VEHICLES" ? t(`ADD_ASSUMPTION_${category}`) : t("ADD_ASSUMPTION")} onClick={() => setAssumptionsPopUp(true)} variation="secondary" - // isDisabled={isAddNewDisabled} - isDisabled={false} + isDisabled={isAddNewDisabled} />🧰 Tools
🪛 Biome
[error] 160-160: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
Line range hint
82-117: Refactor addNewAssumption for better maintainabilityThe
addNewAssumptionfunction contains duplicate logic patterns for different cases. Consider extracting common logic into helper functions.Consider refactoring like this:
+ const addAssumptionToState = (key, source = "CUSTOM") => { + setAssumptions(prev => [...prev, key]); + if (!assumptionValues.some(assumption => assumption.key === key)) { + setAssumptionValues(prev => [...prev, { + source, + key, + value: null, + category + }]); + } + }; const addNewAssumption = () => { if (category === "CAMPAIGN_VEHICLES") { - const assumptionToAdd = { - ...selectedDeletedAssumption, - source: "CUSTOM", - category: category, - value: null, - key: selectedDeletedAssumption?.model, - }; - setAssumptions([...assumptions, assumptionToAdd?.key]); - if (!assumptionValues.some((assumption) => assumption.key === assumptionToAdd)) { - setAssumptionValues((prevValues) => [ - ...prevValues, - { source: "CUSTOM", key: selectedDeletedAssumption?.model, value: null, category: category }, - ]); - } + addAssumptionToState(selectedDeletedAssumption?.model); } else if (selectedDeletedAssumption?.code === "NEW_ASSUMPTION") { - const assumptionToAdd = selectedDeletedAssumption; - setAssumptions([...assumptions, assumptionToAdd?.name]); - if (!assumptionValues.some((assumption) => assumption.key === assumptionToAdd)) { - setAssumptionValues((prevValues) => [ - ...prevValues, - { source: "CUSTOM", key: assumptionToAdd?.name, value: null, category: category }, - ]); - } + addAssumptionToState(selectedDeletedAssumption?.name); } else if (selectedDeletedAssumption) { const assumptionToAdd = deletedAssumptions.find( (assumption) => assumption === selectedDeletedAssumption.code ); if (assumptionToAdd && !assumptions.includes(assumptionToAdd)) { - setAssumptions([...assumptions, assumptionToAdd]); setDeletedAssumptions(deletedAssumptions.filter( (assumption) => assumption !== selectedDeletedAssumption.code )); if (deletedAssumptionCategories.current[category]) { deletedAssumptionCategories.current[category] = deletedAssumptionCategories.current[category].filter( (item) => item !== assumptionToAdd ); } - if (!assumptionValues.some((assumption) => assumption.key === assumptionToAdd)) { - setAssumptionValues((prevValues) => [ - ...prevValues, - { source: "MDMS", key: assumptionToAdd, value: null }, - ]); - } + addAssumptionToState(assumptionToAdd, "MDMS"); } } setSelectedDeletedAssumption(null); setAssumptionsPopUp(false); };🧰 Tools
🪛 Biome
[error] 160-160: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
Based on the codebase analysis, I can now provide the final response:
Add validation for assumption name using existing validation utilities
The codebase already has a robust validation pattern in
utilities.jsthat can be leveraged:
- Use
validateFormData()for empty string checks- Implement a pattern validation similar to
validateName()used for microplan namesExample locations that need validation:
- Line 257:
onChange={(e) => setSelectedDeletedAssumption(e.target.value)}- Line 281:
onChange={(e) => setSelectedDeletedAssumption(e.target.value)}🔗 Analysis chain
Line range hint
269-275: Add validation for assumption nameThe assumption name input field lacks validation. Consider adding checks for empty strings, special characters, or duplicate names.
Let's verify if there are any validation patterns used elsewhere in the codebase:
Would you like me to help implement the validation logic for the assumption name?
Let me search for validation patterns specifically in the microplan module and related components.
Let me search for the validation patterns specifically used in the Hypothesis component and related files.
Let me try one final search to find any validation patterns in the microplan module's utilities or common functions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for input validation patterns rg -A 3 'TextInput.*name.*value.*onChange'Length of output: 4470
Script:
#!/bin/bash # Search for validation patterns in microplan module rg -A 3 'validation|validate|isValid' health/micro-ui/web/micro-ui-internals/packages/modules/microplan/ # Search for existing assumption name validations ast-grep --pattern 'setSelectedDeletedAssumption($$$)'Length of output: 46541
Script:
#!/bin/bash # Search for validation patterns in Hypothesis.js and related files rg -A 5 'validateName|validation|pattern' health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js # Search for assumption-related validation patterns ast-grep --pattern 'function validateAssumption($$$)'Length of output: 194
Script:
#!/bin/bash # Search for validation utilities in the microplan module rg -A 5 'validate|pattern|isValid' health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/ # Search for assumption validation in the entire codebase ast-grep --pattern 'assumption.*validate'Length of output: 8272
🧰 Tools
🪛 Biome
[error] 160-160: 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/microplan/src/components/Hypothesis.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1)
Pattern **/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
[error] 160-160: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/fetchDataAndSetParams.js (1)
Line range hint
1-200: Consider refactoring for improved maintainability.While not directly related to the current changes, the
fetchDataAndSetParamsfunction could benefit from some architectural improvements:
Consider breaking down the function into smaller, focused utilities:
- Boundary data processing
- File handling
- Assumptions processing
- Formula configuration
Consider introducing explicit error handling instead of relying solely on optional chaining.
This would improve maintainability, testability, and make future changes easier to implement.
Would you like me to provide a detailed refactoring proposal?
📜 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/utils/fetchDataAndSetParams.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/fetchDataAndSetParams.js (1)
Pattern **/*.js: check
🔇 Additional comments (1)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/utils/fetchDataAndSetParams.js (1)
58-58: LGTM! Verify locale utility availability.
The change improves i18n key handling by using the locale transformation utility. This is a good practice for consistent internationalization.
Let's verify the Digit.Utils.locale dependency and its usage:
✅ Verification successful
Based on the search results, I can see that:
Digit.Utils.locale.getTransformedLocaleis widely used across the codebase for i18n key transformations- The utility is imported via various
@egovernments/digit-ui-*packages - The implementation exists in
micro-ui/web/micro-ui-internals/packages/libraries/src/utils/locale.js
Let me check the actual implementation to verify if this is a safe change:
The locale transformation change is safe and consistent with codebase practices
The implementation of getTransformedLocale in the codebase shows that it's a standardized utility function that:
- Converts input to uppercase
- Replaces special characters (dots, colons, hyphens, spaces, slashes) with underscores
- Handles null safety and number inputs
The change in the review file follows the established pattern of using this utility for i18n key normalization, which is widely used across the codebase for consistent internationalization handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for Digit.Utils.locale usage and imports
# Test 1: Look for other usages of getTransformedLocale
rg "Digit\.Utils\.locale\.getTransformedLocale" -A 2
# Test 2: Check for Digit utils import or definition
rg "import.*Digit" -A 2
Length of output: 71001
Script:
#!/bin/bash
# Check the implementation of getTransformedLocale
cat micro-ui/web/micro-ui-internals/packages/libraries/src/utils/locale.js
Length of output: 3693
Summary by CodeRabbit
New Features
Bug Fixes
Style