added css fixes #1710
added css fixes #1710
Conversation
📝 Walkthrough📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily involve updates to various CSS and SCSS files, enhancing the styling and responsiveness of UI components. Notably, the Changes
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 (
|
* updated upload facility and population css * updated assumption screen css --------- Co-authored-by: rachana-egov <rachana.singh@egovernment.org>
![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 (13)
health/micro-ui/web/public/index.html (1)
Line range hint 1-35: Consider performance optimizations for resource loading.
To improve page load performance, consider:
- Adding preconnect hints for the unpkg.com domain
- Bundling multiple digit-ui CSS files together to reduce HTTP requests
Add these lines in the <head> section:
+ <link rel="preconnect" href="https://unpkg.com">
+ <link rel="preconnect" href="https://fonts.googleapis.com">Consider implementing a strategy to:
- Bundle the CSS files during build time instead of loading them individually from CDN
- Implement a consistent versioning strategy across all digit-ui packages to avoid potential compatibility issues between beta and stable versions
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BoundarySelection.js (1)
Line range hint 2-2: Consider consolidating Card imports
There are two Card imports from different packages which could lead to confusion. If you're migrating to @egovernments/digit-ui-components, consider removing the unused import from digit-ui-react-components.
import { CardText, Header } from "@egovernments/digit-ui-react-components";
-import { Card } from "@egovernments/digit-ui-react-components";
import { Card } from "@egovernments/digit-ui-components";Also applies to: 7-7
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanDetails.js (1)
Line range hint 90-126: Clean up code and improve styling practices.
- Remove commented out TextInput code block
- Consider moving inline styles to CSS classes for better maintainability
- {/* <TextInput
- t={t}
- style={{ width: "100%", margin: 0 }}
- type={"text"}
- isMandatory={false}
- name="name"
- value={microplan}
- onChange={onChangeMicroplanName}
- placeholder={t("MICROPLAN_NAME_INPUT_PLACEHOLDER")}
- disable={isFreezed}
- /> */}Consider moving inline styles to CSS classes:
-<CardNew
- style={{
- marginTop: "1.5rem",
- }}
+<CardNew
+ className="microplan-name mt-6"
>health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (1)
Line range hint 87-141: Refactor repeated patterns into a reusable component.
The code contains three nearly identical LabelFieldPair sections with duplicated styles and structure. Consider creating a reusable component to reduce duplication and improve maintainability.
const CampaignField = ({ label, options, selected, onSelect, disabled, optionKey }) => (
<LabelFieldPair className="campaign-field-pair">
<div className="campaign-type-label-pair">
<span className="campaign-type-label">{label}</span>
<span className="mandatory-span">*</span>
</div>
<Dropdown
className="campaign-dropdown"
t={t}
option={options}
optionKey={optionKey}
selected={selected}
select={onSelect}
disabled={disabled}
/>
</LabelFieldPair>
);Then use it like:
<CampaignField
label={t("HCM_DISEASE_TYPE")}
options={data?.diseases}
optionKey="code"
selected={disease}
onSelect={setDisease}
disabled={isFreezed}
/>Also, move the common styles to CSS:
.campaign-field-pair {
margin-bottom: 0;
}
.campaign-type-label-pair {
min-width: 17rem;
}
.campaign-dropdown {
width: 40rem;
}health/micro-ui/web/micro-ui-internals/packages/css/tailwind.config.js (2)
Line range hint 120-138: Consider adjusting similar chart colors for better distinction.
The chart colors are well-defined, but chart-1 (#048BD0) and chart-5 (#0BABDE) are very similar shades of blue, which might be difficult to distinguish in data visualizations.
Consider adjusting one of these colors for better visual distinction:
chart: {
"chart-1": "#048BD0",
"chart-1-gradient": "#048BD0",
"chart-2": "#FBC02D",
"chart-2-gradient": "#FBC02D",
"chart-3": "#8E29BF",
"chart-4": "#EA8A3B",
- "chart-5": "#0BABDE",
+ "chart-5": "#2ECC71", // Example: Using a distinct green shade
},Line range hint 139-219: LGTM! Consider adding documentation for the typography system.
The typography system is well-structured with a comprehensive mobile-first approach and consistent scaling. The line heights are appropriately proportional to font sizes across breakpoints.
Consider adding a comment block at the top of the typography section to document the scaling system and usage guidelines:
+ /**
+ * Typography Scale
+ *
+ * Follows a mobile-first responsive approach:
+ * - xs: 0.75rem (12px)
+ * - s: 0.875rem - 1rem (14px - 16px)
+ * - m: 1rem - 1.25rem (16px - 20px)
+ * - l: 1.25rem - 1.5rem (20px - 24px)
+ * - xl: 1.5rem - 2.5rem (24px - 40px)
+ *
+ * Usage:
+ * text-body-s-mobile -> 0.875rem
+ * text-body-s-tablet -> 1rem
+ * text-body-s-desktop -> 1rem
+ */
fontSize: {
"heading-xl": {
mobile: "2rem",health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (3)
Line range hint 1-445: Consider architectural improvements for better maintainability
While reviewing the component holistically, here are some suggestions to improve the code architecture:
- Consider breaking down this large component into smaller, focused components
- Consolidate state management using a reducer pattern
- Extract form validation logic into a separate hook
- Combine related useEffect hooks to reduce complexity
Here's a suggested approach for breaking down the component:
// StepperNavigation.js
const StepperNavigation = ({ currentStep, active, steps }) => {
return (
<Stepper
customSteps={steps}
onStepClick={() => null}
currentStep={currentStep + 1}
activeSteps={active}
/>
);
};
// FormSection.js
const FormSection = ({ config, onSubmit, onBack, nextLabel }) => {
return (
<FormComposerV2
config={config}
onSubmit={onSubmit}
showSecondaryLabel={true}
secondaryLabel={t("MP_BACK")}
actionClassName={"actionBarClass"}
className="setup-campaign"
cardClassName="setup-campaign-card"
noCardStyle={true}
onSecondayActionClick={onBack}
label={nextLabel}
/>
);
};
// AlertPopup.js
const AlertPopup = ({ show, config, onConfirm, onCancel }) => {
if (!show) return null;
return (
<PopUp
alertHeading={t(config.alertHeader)}
equalWidthButtons={true}
className="alert-popup-setup-microplan"
// ... other props
>
<div>{t(config.alertMessage)}</div>
</PopUp>
);
};Would you like me to provide more detailed examples of these architectural improvements?
Line range hint 34-35: Consider using a custom hook for navigation state management
The navigation state (currentStep and currentKey) and their update logic could be extracted into a custom hook to improve reusability and maintainability.
const useStepNavigation = (initialKey = 1) => {
const [currentStep, setCurrentStep] = useState(0);
const [currentKey, setCurrentKey] = useState(initialKey);
const goToNextStep = () => {
setCurrentStep(prev => prev + 1);
setCurrentKey(prev => prev + 1);
};
const goToPreviousStep = () => {
setCurrentStep(prev => prev - 1);
setCurrentKey(prev => prev - 1);
};
return {
currentStep,
currentKey,
goToNextStep,
goToPreviousStep,
setCurrentStep,
setCurrentKey
};
};Also applies to: 45-46
Line range hint 272-291: Consolidate event listeners setup
The event listeners for navigation could be consolidated into a single useEffect to improve maintainability.
useEffect(() => {
const eventHandlers = {
moveToPrevious: moveToPreviousStep,
revertToPreviousScreenFromFormula: goToPreviousScreenFromFormula,
};
// Setup all event listeners
Object.entries(eventHandlers).forEach(([event, handler]) => {
window.addEventListener(event, handler);
});
// Cleanup all event listeners
return () => {
Object.entries(eventHandlers).forEach(([event, handler]) => {
window.removeEventListener(event, handler);
});
};
}, []);health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaryComponent.js (4)
Line range hint 32-49: Add PropTypes validation for component props.
Both Wrapper and SelectingBoundaryComponent accept multiple props without type validation. Consider adding PropTypes to improve type safety and documentation.
Add PropTypes validation:
import PropTypes from 'prop-types';
Wrapper.propTypes = {
hierarchyType: PropTypes.string.isRequired,
lowest: PropTypes.string.isRequired,
frozenData: PropTypes.array,
frozenType: PropTypes.string,
selectedData: PropTypes.array,
onSelect: PropTypes.func.isRequired,
boundaryOptions: PropTypes.object,
updateBoundary: PropTypes.func,
hierarchyData: PropTypes.array,
isMultiSelect: PropTypes.bool,
restrictSelection: PropTypes.bool
};Also applies to: 50-68
🧰 Tools
🪛 Biome
[error] 472-472: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 470-470: 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)
Line range hint 471-502: Consider extracting boundary selection logic into separate hooks.
The boundary selection logic is complex and handles multiple responsibilities. Consider breaking it down into custom hooks for better maintainability and testing.
Extract the boundary selection logic into a custom hook:
const useBoundarySelection = (options) => {
const [selectedData, setSelectedData] = useState([]);
const [boundaryOptions, setBoundaryOptions] = useState({});
const handleBoundaryChange = useCallback((data, boundary) => {
// ... existing logic
}, [/* dependencies */]);
return {
selectedData,
boundaryOptions,
handleBoundaryChange
};
};🧰 Tools
🪛 Biome
[error] 472-472: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 470-470: 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)
Line range hint 508-568: Add key prop to mapped elements and improve accessibility.
The mapped elements in the render method are missing key props, and the mandatory field indicators could be more accessible.
Apply these improvements:
<LabelFieldPair
+ key={boundary.boundaryType}
style={{ alignItems: "flex-start", paddingRight: "30%" }}>
<CardLabel className={"boundary-selection-label"}>
{t((hierarchyType + "_" + boundary?.boundaryType).toUpperCase())}
- <span className="mandatory-span">*</span>
+ <span className="mandatory-span" aria-label="required field">*</span>
</CardLabel>🧰 Tools
🪛 Biome
[error] 509-509: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 507-507: 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)
Line range hint 583-607: Optimize performance with useMemo and useCallback.
The component performs complex computations during render and has multiple effects that might cause unnecessary re-renders.
Consider these optimizations:
// Memoize complex computations
const boundaryOptions = useMemo(() =>
Object.entries(options)
.filter(([key]) => key.startsWith(boundary.boundaryType))
.flatMap(([key, value]) =>
Object.entries(value || {}).map(([subkey, item]) => ({
code: item?.split(".")?.[0],
name: subkey,
type: boundary.boundaryType,
}))
), [options, boundary.boundaryType]);
// Memoize callback functions
const handleSelect = useCallback((value) => {
handleBoundaryChange(value, boundary);
}, [handleBoundaryChange, boundary]);🧰 Tools
🪛 Biome
[error] 584-584: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
[error] 582-582: 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)
📜 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 (13)
- 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/css/src/pages/employee/facility.scss (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/index.scss (0 hunks)
- health/micro-ui/web/micro-ui-internals/packages/css/tailwind.config.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js (5 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaryComponent.js (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BoundarySelection.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (5 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanDetails.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UserManagementConfig.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1 hunks)
- health/micro-ui/web/public/index.html (1 hunks)
💤 Files with no reviewable changes (1)
- health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/index.scss
🧰 Additional context used
📓 Path-based instructions (8)
health/micro-ui/web/micro-ui-internals/packages/css/tailwind.config.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/components/SelectingBoundaryComponent.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BoundarySelection.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanDetails.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UserManagementConfig.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
Pattern **/*.js: check
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/SelectingBoundaryComponent.js
[error] 507-507: 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 (17)
health/micro-ui/web/micro-ui-internals/example/public/index.html (1)
19-19: Document the rationale for version update.
Please update the PR description to include:
- What CSS fixes were implemented in this version?
- Which UI components are affected?
- Are there any visual changes that need to be verified?
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/BoundarySelection.js (2)
Line range hint 98-113: Ensure visual consistency across devices
Since this change affects the layout of a critical selection component, please:
- Test the component's appearance across different screen sizes
- Verify that the new styling doesn't break the popup's layout
- Consider adding before/after screenshots to the PR description
98-100: Verify CSS class definitions
The changes introduce new CSS classes. Please ensure that selecting-boundary-card and boundary-selection-description are properly defined in your stylesheet.
✅ Verification successful
CSS classes are properly defined
The CSS classes selecting-boundary-card and boundary-selection-description are correctly defined in health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss. The styling is consistent with the component's usage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the new CSS class definitions
rg "selecting-boundary-card|boundary-selection-description" --type css --type scss
Length of output: 113
Script:
#!/bin/bash
# Search for CSS class definitions with broader file patterns and context
rg "selecting-boundary-card|boundary-selection-description" -g "*.css" -g "*.scss" -g "*.less" -g "*.module.css" -A 2
# Also search in the same directory as the component for any style files
fd -e css -e scss -e less . health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/
# Check if styles might be defined in a styles.js or similar file
rg "selecting-boundary-card|boundary-selection-description" -g "*style*.js" -g "*Style*.js" -A 2
Length of output: 808
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss (1)
30-50: LGTM! Well-structured responsive typography.
The implementation follows best practices with proper use of theme variables and consistent breakpoints.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanDetails.js (2)
15-16: LGTM: Import statements are properly structured.
The new imports are correctly organized and sourced from the appropriate package.
130-130: Verify responsive behavior with unset maxWidth.
Setting maxWidth: "unset" might affect the component's responsive behavior on different screen sizes. Consider using a specific max-width value or CSS classes for better control over the layout.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UserManagementConfig.js (1)
132-132: Verify the icon component change and its existence.
The change from "TqmInboxIcon" to "Population" raises some concerns:
- The new component name doesn't follow the typical icon naming convention (usually ends with 'Icon')
- The icon might not semantically represent user management functionality
- This change seems unrelated to the PR's purpose of CSS fixes
Let's verify if the "Population" component exists and is properly imported:
✅ Verification successful
Let me verify the existence and proper import of the Population component with one more search:
Let me check one final thing - the imports in the UserManagementConfig file:
The Population component change is valid and properly implemented.
The verification results show that:
- The Population component exists and is properly defined in the svg-components package
- It's a valid SVG icon component that's exported and available for use
- The component is being used correctly as a logoIcon in the UserManagementConfig
While the component name doesn't follow the typical "Icon" suffix convention, it's part of the official SVG component library and represents a group of people, which is semantically appropriate for a User Management interface.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for Population component definition or import
echo "Searching for Population component definition..."
ast-grep --pattern 'export $_ Population = $_'
echo "Searching for Population component imports..."
rg "import.*Population.*from"
echo "Searching for Population component usage..."
rg "component.*:.*['\"]Population['\"]"
Length of output: 1320
Script:
#!/bin/bash
# Check if Population component is defined in SVG components
echo "Checking Population SVG component definition..."
cat micro-ui/web/micro-ui-internals/packages/svg-components/src/svg/Population.js 2>/dev/null || echo "File not found"
# Check if Population is exported from svg-components
echo -e "\nChecking svg-components exports..."
cat micro-ui/web/micro-ui-internals/packages/svg-components/src/index.js 2>/dev/null || echo "File not found"
# Check if svg-components is imported in the UserManagementConfig
echo -e "\nChecking imports in UserManagementConfig..."
rg "import.*from.*svg-components" health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UserManagementConfig.js
Length of output: 52476
Script:
#!/bin/bash
# Check the full content of UserManagementConfig.js to see imports
echo "Checking UserManagementConfig.js content..."
cat health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/configs/UserManagementConfig.js
Length of output: 5569
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js (2)
Line range hint 10-143: Overall implementation is solid and well-structured.
The component demonstrates good practices:
- Proper state management
- Internationalization support
- Error handling
- Loading states
The recent changes improve the structure while maintaining the core functionality.
4-7: Verify consistency of package migration across the codebase.
The change from @egovernments/digit-ui-react-components to @egovernments/digit-ui-components appears to be part of a package migration. Let's ensure this pattern is consistent across the codebase.
health/micro-ui/web/micro-ui-internals/packages/css/tailwind.config.js (1)
Line range hint 1-219: LGTM! Well-structured configuration with clean organization.
The configuration maintains a clear separation between Tailwind's default theme and the custom digitv2 namespace. All new additions are consistently organized and follow Tailwind's configuration patterns.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/pages/employee/SetupMicroplan.js (1)
412-412: LGTM: CSS class addition for popup styling
The addition of alert-popup-setup-microplan class aligns with the CSS fixes mentioned in the PR objectives.
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (3)
2552-2553: LGTM! Visual improvements to KPI container.
The changes enhance the visual hierarchy by adding a subtle box-shadow and adjusting the spacing for a more compact layout.
2562-2562: LGTM! Improved typography and theming.
Good use of typography extension and theme variables for better maintainability and consistency with the design system.
Also applies to: 2564-2564
2569-2570: LGTM! Enhanced paragraph styling.
Good use of typography extension and theme variable for secondary text, maintaining consistency with the design system.
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js (3)
392-395: LGTM!
The useEffect hook correctly resets optionIndex when searchQuery changes, ensuring proper navigation.
887-889: Verify the necessity of passing 'frozenType' prop to 'Wrapper' component
Ensure that the frozenType prop passed to the Wrapper component is utilized within it. If it's not used, consider removing it to keep the code clean.
801-801:
Conditionally render the search input based on 'isSearchable' prop
The search input is currently rendered regardless of the isSearchable prop. It should be conditionally rendered only when isSearchable is true to prevent unintended behavior.
Apply this diff to conditionally render the search input:
{/* Existing code */}
- <input
- className="digit-cursorPointer"
- style={{}}
- type="text"
- onKeyDown={keyChange}
- onFocus={() => setActive(true)}
- value={searchQuery}
- onChange={onSearch}
- />
+ {isSearchable && (
+ <input
+ className="digit-cursorPointer"
+ style={{}}
+ type="text"
+ onKeyDown={keyChange}
+ onFocus={() => setActive(true)}
+ value={searchQuery}
+ onChange={onSearch}
+ />
+ )}Likely invalid or redundant comment.
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/facility.scss
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/MicroplanDetails.js
Show resolved
Hide resolved
...micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/CampaignDetails.js
Show resolved
Hide resolved
...b/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js
Show resolved
Hide resolved
...b/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js
Show resolved
Hide resolved
...b/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 28
🧹 Outside diff range comments (6)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1)
Line range hint 91-111: Extract validation logic into a reusable function
The validation logic is duplicated between handleNext and handleStepClick. Consider extracting it into a separate function for better maintainability and reusability.
+ const validateAssumptions = (currentAssumptions) => {
+ const existingAssumptionKeys = assumptionValues?.map(assumption => assumption.key);
+ const visibleAssumptions = currentAssumptions.filter(item =>
+ existingAssumptionKeys?.includes(item) && !deletedAssumptions?.includes(item)
+ );
+
+ const hasEmptyFields = visibleAssumptions.some(item => {
+ const value = assumptionValues.find(assumption => assumption.key === item)?.value;
+ return !value;
+ });
+
+ if (hasEmptyFields) {
+ setShowToast({
+ key: "error",
+ label: t("ERR_MANDATORY_FIELD"),
+ transitionTime: 3000,
+ });
+ }
+
+ return !hasEmptyFields;
+ };
const handleNext = () => {
const currentAssumptions = assumptionCategories[internalKey - 1]?.assumptions || [];
- const existingAssumptionKeys = assumptionValues?.map(assumption => assumption.key);
- const visibleAssumptions = currentAssumptions.filter(item =>
- existingAssumptionKeys?.includes(item) && !deletedAssumptions?.includes(item)
- );
-
- const hasEmptyFields = visibleAssumptions.some(item => {
- const value = assumptionValues.find(assumption => assumption.key === item)?.value;
- return !value;
- });
-
- if (hasEmptyFields) {
- setShowToast({
- key: "error",
- label: t("ERR_MANDATORY_FIELD"),
- transitionTime: 3000,
- });
- return;
- }
+ if (!validateAssumptions(currentAssumptions)) return;health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
Line range hint 284-290: Simplify assumptions filtering logic
The current implementation can be made more concise while maintaining the same functionality.
Consider this simplified version:
-if (assumptionsInPlan?.length > 0) {
- const filteredAssumptions = assumptionsInPlan?.filter((assumption) => assumption.category === currentCategory);
- const keys = filteredAssumptions?.map((assumption) => assumption.key);
- setAssumptions(keys);
-}
+const keys = assumptionsInPlan
+ ?.filter(assumption => assumption.category === currentCategory)
+ ?.map(assumption => assumption.key) ?? [];
+setAssumptions(keys);health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (2)
Line range hint 349-394: Improve error handling in file upload process.
The file upload process could benefit from more structured error handling and clearer separation of concerns.
+ const validateFileUpload = (file) => {
+ if (file.length > 1) {
+ throw new Error(t("HCM_ERROR_MORE_THAN_ONE_FILE"));
+ }
+ };
+ const uploadFile = async (file, module) => {
+ const { data: { files: fileStoreIds } = {} } = await Digit.UploadServices.MultipleFilesStorage(module, file, tenantId);
+ if (!fileStoreIds || fileStoreIds.length === 0) {
+ throw new Error(t("HCM_ERROR_FILE_UPLOAD_FAILED"));
+ }
+ return fileStoreIds;
+ };
const onBulkUploadSubmit = async (file) => {
try {
- if (file.length > 1) {
- setShowToast({ key: "error", label: t("HCM_ERROR_MORE_THAN_ONE_FILE") });
- return;
- }
+ validateFileUpload(file);
setFileName(file?.[0]?.name);
const module = "HCM-ADMIN-CONSOLE-CLIENT";
- const { data: { files: fileStoreIds } = {} } = await Digit.UploadServices.MultipleFilesStorage(module, file, tenantId);
- if (!fileStoreIds || fileStoreIds.length === 0) {
- throw new Error(t("HCM_ERROR_FILE_UPLOAD_FAILED"));
- }
+ const fileStoreIds = await uploadFile(file, module);Line range hint 507-519: Improve error message parsing logic.
The error parsing logic could be more robust with a dedicated error handling utility.
+ const parseErrorMessage = (error) => {
+ try {
+ const parsedError = JSON.parse(error);
+ return parsedError?.description || parsedError?.message || t("HCM_VALIDATION_FAILED");
+ } catch (e) {
+ console.error("Error parsing JSON:", e);
+ return t("HCM_VALIDATION_FAILED");
+ }
+ };
if (temp?.status == "failed" && temp?.additionalDetails?.error) {
- try {
- const parsedError = JSON.parse(temp.additionalDetails.error);
- const errorMessage = parsedError?.description || parsedError?.message || t("HCM_VALIDATION_FAILED");
- setShowToast({ key: "error", label: errorMessage, transitionTime: 5000000 });
- } catch (e) {
- console.error("Error parsing JSON:", e);
- setShowToast({ key: "error", label: t("HCM_VALIDATION_FAILED"), transitionTime: 5000000 });
- }
+ const errorMessage = parseErrorMessage(temp.additionalDetails.error);
+ setShowToast({ key: "error", label: errorMessage, transitionTime: 5000000 });
}health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (2)
Line range hint 2677-2688: Optimize loader overlay performance.
The loader overlay implementation could be optimized for better performance.
Consider these improvements:
.loader-overlay {
position: fixed;
top: 0;
left: 0;
right: 0;
bottom: 0;
- background-color: rgba(255, 255, 255, 0.7);
+ backdrop-filter: blur(5px);
display: flex;
justify-content: center;
align-items: center;
z-index: 9999;
pointer-events: all;
+ // Add will-change for performance
+ will-change: opacity;
+ // Add transition for smooth appearance
+ transition: opacity 0.2s ease-in-out;
}Line range hint 2733-2739: Fix duplicate property in button label styling.
There's a duplicate min-width: max-content declaration in the button label styles.
Remove the duplicate property:
.microplan-response-button {
.icon-label-container {
.digit-button-label {
- min-width: max-content;
min-width: max-content;
}
}
}📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (8)
- health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (34 hunks)
- health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/index.scss (4 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/XlsPreview.js (3 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/AssumptionsForm.js (6 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (6 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (8 hunks)
- health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (20 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/XlsPreview.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/AssumptionsForm.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1)
Pattern **/*.js: check
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1)
Pattern **/*.js: check
📓 Learnings (1)
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss (3)
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#876
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:1940-2392
Timestamp: 2024-06-14T14:10:38.086Z
Learning: Classes related to interactive elements in the microplan preview section are mostly passed to Higher Order Components (HOCs), and ARIA attributes for non-HOC elements will be managed directly by adding them where necessary.
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#876
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:1940-2392
Timestamp: 2024-10-08T20:11:07.772Z
Learning: Classes related to interactive elements in the microplan preview section are mostly passed to Higher Order Components (HOCs), and ARIA attributes for non-HOC elements will be managed directly by adding them where necessary.
Learnt from: siddhant-nawale-egov
PR: egovernments/DIGIT-Frontend#876
File: micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss:1940-2392
Timestamp: 2024-10-08T20:11:12.539Z
Learning: Classes related to interactive elements in the microplan preview section are mostly passed to Higher Order Components (HOCs), and ARIA attributes for non-HOC elements will be managed directly by adding them where necessary.
🪛 Biome
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/AssumptionsForm.js
[error] 30-30: 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] 171-202: 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] 189-199: 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/components/Hypothesis.js
[error] 142-142: 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] 164-199: 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] 201-251: 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] 169-169: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 170-170: 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] 178-186: 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] 187-193: 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] 206-206: Avoid passing children using a prop
The canonical way to pass children in React is to use JSX elements
(lint/correctness/noChildrenProp)
[error] 207-221: 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] 227-235: 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] 236-244: 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/components/HypothesisWrapper.js
[error] 45-45: 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)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
[error] 741-747: 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 (9)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/XlsPreview.js (4)
22-27: LGTM! Clean array structure.
The array structure has been simplified while maintaining the same functionality.
43-45: Visual hierarchy improved with primary button.
The download button's prominence has been increased appropriately by changing to primary variation. The icon positioning is now controlled via isSuffix.
35-38: Verify the new icon system implementation.
The icon prop has changed from a JSX element to a string identifier ("ArrowBack"). While this is a cleaner approach:
#!/bin/bash
# Description: Verify the new icon system implementation
# Check if other components use the same pattern and if the icon is properly registered
echo "Checking Button usage with string-based icons:"
rg -A 2 "Button.*icon=['\"]ArrowBack['\"]"
echo -e "\nChecking if ArrowBack icon is registered:"
rg "ArrowBack.*icon" --type js55-55: Verify contrast ratio with new white text color.
The text color has been changed to white (#FFFFFF). Please ensure this provides sufficient contrast with the background color for accessibility.
#!/bin/bash
# Description: Look for background color definitions that might affect contrast
echo "Checking for background color definitions in related CSS/SCSS files:"
rg -t css -t scss "background-color|background:" "$(dirname $(fd -t f "XlsPreview" | head -n1))"health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/HypothesisWrapper.js (1)
Line range hint 1-351: Verify component behavior with different assumption configurations
Please ensure thorough testing of the component with:
- Different campaign types and resource distribution strategies
- Empty and filled assumption values
- Navigation between steps with validation
- Error scenarios and toast notifications
✅ Verification successful
Let me gather more information about the usage context and test coverage.
Based on the gathered information, I can now provide a final response:
Component changes are well-contained with proper error handling
The verification shows that:
- HypothesisWrapper is used in a controlled manner within the microplan module
- The component has proper error handling with Toast notifications
- Changes are localized to the hypothesis/assumptions flow
- The component follows the same error handling patterns as other components in the codebase
The review comment's concerns about testing different configurations are addressed by the existing validation and error handling mechanisms in the code. The component already handles:
- Empty field validation
- Step navigation restrictions
- Toast notifications for errors
- Different campaign types and distribution strategies through the filteredData logic
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other components using HypothesisWrapper to understand the impact
rg -l "HypothesisWrapper" --type js
# Look for test files related to this component
fd -e test.js -e spec.js hypothesis
Length of output: 452
Script:
#!/bin/bash
# Check how HypothesisWrapper is used in other files
rg -A 5 "HypothesisWrapper" --type js
# Check for any existing test setup or mocks
rg -l "test|spec|mock" --type js --glob "*hypothesis*" --glob "*Hypothesis*"
# Look for any error handling patterns
rg "setShowToast|Toast" --type js health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/
Length of output: 41373
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfigWrapper.js (1)
361-366: Verify styling consistency with design system
The gap reduction and class name changes suggest a shift in the stepper layout. Ensure these changes align with the design system guidelines.
✅ Verification successful
Styling changes are consistent with design system
The gap reduction from 2rem to 1.5rem and the use of vertical-stepper-card class are consistent with the established styling patterns in the codebase. The evidence shows:
- The
vertical-stepper-cardclass is properly defined inindex.scsswith consistent height (36.5rem) and styling - The 1.5rem gap is commonly used throughout the codebase for consistent spacing
- The class is used consistently across multiple components in the microplan module
The changes maintain visual consistency with the rest of the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of vertical-stepper-card to ensure consistent styling
rg "vertical-stepper-card" --type css --type scss
Length of output: 78
Script:
#!/bin/bash
# Search for vertical-stepper-card class usage in all files
rg "vertical-stepper-card"
# Search for similar stepper card styling patterns
rg "stepper.*card"
# Look for any CSS/SCSS files that might contain stepper styles
fd "\.(css|scss)$" --exec grep -l "stepper"
Length of output: 3599
Script:
#!/bin/bash
# Check the styling definitions in the identified CSS files
cat health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/index.scss
cat health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/campaign.scss
Length of output: 23372
health/micro-ui/web/micro-ui-internals/packages/css/src/pages/employee/index.scss (2)
100-104: LGTM! Good use of typography extension and theme variables.
The class follows best practices by extending typography styles and using theme variables for colors.
993-1002: 🧹 Nitpick (assertive)
LGTM! Minor formatting suggestion.
The class uses proper padding values and theme variables. However, there's an empty line between properties that breaks the consistency.
.card-header-timeline {
padding: .75rem;
padding-left: 1.5rem;
padding-right: 1.5rem;
border: 0.063rem solid;
align-items: center;
color: theme(digitv2.lightTheme.header-sidenav) !important;
.digit-text-block-subheader {
color: theme(digitv2.lightTheme.header-sidenav) !important;
}
}Likely invalid or redundant comment.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js (1)
349-362: 🛠️ Refactor suggestion
Add file type validation for security.
Consider adding file type validation before processing uploads to prevent security vulnerabilities.
+ const ALLOWED_FILE_TYPES = ['application/vnd.openxmlformats-officedocument.spreadsheetml.sheet', 'application/vnd.ms-excel'];
+ const validateFileType = (file) => {
+ if (!ALLOWED_FILE_TYPES.includes(file.type)) {
+ throw new Error(t("HCM_ERROR_INVALID_FILE_TYPE"));
+ }
+ };
const onBulkUploadSubmit = async (file) => {
try {
validateFileUpload(file);
+ validateFileType(file[0]);
setFileName(file?.[0]?.name);✅ Verification successful
Let me analyze the file upload validation across the codebase to make a proper assessment.
Based on the findings from both searches, I can now provide a final response:
Add file type validation to prevent security vulnerabilities
The codebase shows inconsistent file type validation. For this HCM admin console upload, implement validation similar to the existing pattern in workbench-hcm:
+ const ALLOWED_FILE_TYPES = ['application/vnd.openxmlformats-officedocument.spreadsheetml.sheet'];
+ const validateFileType = (file) => {
+ if (!ALLOWED_FILE_TYPES.includes(file.type)) {
+ throw new Error(t("HCM_ERROR_ONLY_EXCEL_FILE_ALLOWED"));
+ }
+ };
const onBulkUploadSubmit = async (file) => {
try {
validateFileUpload(file);
+ validateFileType(file[0]);
setFileName(file?.[0]?.name);- Restricts uploads to only Excel (.xlsx) files which is the expected format for this admin console
- Matches the validation pattern used in workbench-hcm module
- Provides clear error message to users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for file type validation in other upload handlers
rg -A 5 "MultipleFilesStorage|Filefetch"
Length of output: 32116
Script:
#!/bin/bash
# Check for any existing file validation in the codebase
rg -A 5 "validateFileType|ALLOWED_FILE_TYPES|file\.(type|mimetype)"
Length of output: 2476
...cro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/XlsPreview.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js
Show resolved
Hide resolved
...icro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/UploadDataCustom.js
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss
Show resolved
Hide resolved
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplanning.scss
Show resolved
Hide resolved
nipunarora-eGov
dismissed
coderabbitai[bot]’s stale review
disminssing since ony css changes
Choose the appropriate template for your PR:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements