-
Notifications
You must be signed in to change notification settings - Fork 20
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BUGFIX/HCMPRE-1585 fixed tootltip icon position according to the label #2010
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe pull request includes updates to several files, primarily focusing on modifying the stylesheet links in HTML files to a newer version of the CSS for the health UI component. Additionally, there are changes in CSS files that adjust layout properties for specific classes and enhancements to rendering logic and error handling in JavaScript components related to formula and assumption management. These changes aim to improve the visual layout and user experience within the health micro UI. 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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range comments (2)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (2)
Line range hint
516-584
: Consider consolidating validation logicThe validation checks for formula names are comprehensive but scattered. Consider extracting them into a separate validation function for better maintainability.
Here's a suggested refactor:
+ const validateFormulaInput = (selectedFormula) => { + if (!selectedFormula) { + return { isValid: false, error: "ERR_FORMULA_SELECT_OPTION" }; + } + if (!selectedFormula?.name && selectedFormula?.code === "NEW_FORMULA") { + return { isValid: false, error: "ERR_FORMULA_ENTER_FORMULA_NAME" }; + } + if (selectedFormula?.code === "NEW_FORMULA" && selectedFormula?.name?.length > 100) { + return { isValid: false, error: "SELECT_FORMULA_NAME_LONG_THAN_100" }; + } + if (category !== "FORMULA_CAMPAIGN_VEHICLES" && filteredFormulas.some((i) => i.output === selectedFormula?.name)) { + return { isValid: false, error: "FORMULA_ALREADY_PRESENT" }; + } + return { isValid: true }; + }; - if(!selectedDeletedFormula){ - setShowToast({ - key: "error", - label: t("ERR_FORMULA_SELECT_OPTION"), - transitionTime: 3000, - style: { zIndex: 1000000 }, - }); - return; - } - // ... other validation checks ... + const validationResult = validateFormulaInput(selectedDeletedFormula); + if (!validationResult.isValid) { + setShowToast({ + key: "error", + label: t(validationResult.error), + transitionTime: 3000, + style: { zIndex: 1000000 }, + }); + return; + }
Line range hint
196-284
: Optimize state management in addNewFormulaThe state management in addNewFormula could be improved to be more efficient and maintainable:
- Consider using useReducer for complex state updates
- Batch related state updates
- Remove commented code to improve readability
Here's a suggested approach:
+ const formulaReducer = (state, action) => { + switch (action.type) { + case 'ADD_CUSTOM_FORMULA': + return { + ...state, + formulas: [...state.formulas, action.payload.formula], + formulaConfigValues: insertFormulaInCategory( + state.formulaConfigValues, + action.payload.newFormula, + action.payload.category + ) + }; + // ... other cases + default: + return state; + } + }; + const [state, dispatch] = useReducer(formulaReducer, { + formulas: initialFormulas, + formulaConfigValues: [], + }); const addNewFormula = () => { if (selectedDeletedFormula?.code === "NEW_FORMULA") { - // Current implementation + dispatch({ + type: 'ADD_CUSTOM_FORMULA', + payload: { + formula: { + source: "CUSTOM", + output: selectedDeletedFormula?.name, + category, + input: "", + operatorName: "", + assumptionValue: "" + }, + category + } + }); } // ... rest of the function };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
⛔ Files ignored due to path filters (1)
health/micro-ui/web/micro-ui-internals/packages/css/package.json
is excluded by!**/*.json
📒 Files selected for processing (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/microplan.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
🔇 Additional comments (3)
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/Hypothesis.js (1)
164-164
: LGTM! Label structure properly updated.
The addition of the wrapper span with the new CSS class properly implements the label styling for consistent alignment with the tooltip icon.
health/micro-ui/web/micro-ui-internals/packages/modules/microplan/src/components/FormulaConfiguration.js (1)
320-320
: LGTM: UI enhancement for tooltip positioning
The addition of the wrapper span with class "assumption-label-icon-wrapper-label" improves the structure and positioning of the formula label and tooltip icon.
health/micro-ui/web/public/index.html (1)
13-13
: LGTM! Version update is consistent with example environment
The CSS version update matches the example environment, maintaining consistency across files.
Let's verify version consistency across all environments:
✅ Verification successful
Version consistency verified across all environments
The CSS version 0.2.2
is consistently used in both production (web/public/index.html
) and example (example/public/index.html
) environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any inconsistent CSS versions across all HTML files
# Find all HTML files and grep for the health CSS package version
fd -e html | xargs grep -l "digit-ui-health-css@" | xargs grep "digit-ui-health-css@"
Length of output: 402
health/micro-ui/web/micro-ui-internals/packages/css/src/components/microplan.scss
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range comments (4)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js (4)
Line range hint
315-324
: Avoid side effects in reducer functionsThe
reducer
function contains side effects such as callingonSelect
,onClose
, andsetSearchQuery("")
, which violates the principles of pure functions in reducers. Reducer functions should be pure and not contain side effects. Consider moving these side effects to auseEffect
hook or handling them after dispatching the action.Apply this diff to remove side effects from the reducer:
function reducer(state, action) { switch (action.type) { case "REMOVE_FROM_SELECTED_EVENT_QUEUE": const newState = state.filter((e) => e?.code !== action.payload?.[1]?.code); - onSelect( - newState.map((e) => e.propsData), - getCategorySelectAllState(), - props - ); - if (onClose && !active) { - setSearchQuery(""); - onClose( - newState.map((e) => e.propsData), - getCategorySelectAllState(), - props - ); - } return newState; case "REPLACE_COMPLETE_STATE": return action.payload; default: return state; } }Then, handle the side effects in a
useEffect
hook:useEffect(() => { onSelect( alreadyQueuedSelectedState?.map((e) => e.propsData), getCategorySelectAllState(), props ); if (onClose && !active) { setSearchQuery(""); onClose( alreadyQueuedSelectedState?.map((e) => e.propsData), getCategorySelectAllState(), props ); } }, [alreadyQueuedSelectedState, active]);
Line range hint
354-364
: Ensure consistent side effect handling when dropdown closesIn the
useEffect
that listens to changes inactive
, you're callingsetSearchQuery("")
,onSelect
, andonClose
whenactive
isfalse
. Ensure that this logic aligns with the intended behavior when the dropdown closes, and avoid redundant or unnecessary calls toonSelect
andonClose
.
Line range hint
525-533
: Use stable keys for list items in chips renderingWhen rendering
Chip
components, usingkey={index}
can lead to rendering issues if the list items change order or if items are added or removed. Ifvalue.code
is unique, consider usingkey={value.code}
to ensure each item has a stable and unique key.Apply this diff:
{items.map((value, index) => { const translatedText = t(value.code); const isClose = frozenData.some((frozenOption) => frozenOption.code === value.code); return ( <Chip - key={index} + key={value.code} text={translatedText?.length > 64 ? `${translatedText.slice(0, 64)}...` : translatedText} onClick={(e) => removeFromDummySelected(e, value)} className="multiselectdropdown-tag" hideClose={isClose} /> ); })}
Line range hint
872-898
: Use unique keys forMenuItem
componentsIn the
MenuItem
component, usingkey={index}
is not recommended because it may cause rendering issues if the list changes. Ifoption.code
is unique, usekey={option.code}
instead to provide a stable identity for each item.Apply this diff:
const MenuItem = ({ option, index }) => { // ... return ( <div - key={index} + key={option.code} className={`digit-multiselectdropodwn-menuitem ${variant ? variant : ""} ${ alreadyQueuedSelectedState.find((selectedOption) => selectedOption.code === option.code) ? "checked" : "" } ${ index === optionIndex && !alreadyQueuedSelectedState.find((selectedOption) => selectedOption.code === option.code) ? "keyChange" : "" }${isFrozen ? "frozen" : ""}`} // ... > {/* ... */} </div> ); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
📒 Files selected for processing (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
health/micro-ui/web/micro-ui-internals/packages/modules/campaign-manager/src/components/MultiSelectDropdown.js (1)
Pattern **/*.js
: check
Choose the appropriate template for your PR:
Feature PR
Feature Request
JIRA ID
Module
Description
Related Issues
Bugfix PR
Bugfix Request
JIRA ID
Module
Description
Root Cause
Related Issues
Release PR
Summary by CodeRabbit
New Features
Bug Fixes
Documentation