Feat: Created shadcn MultiSelectOption comp
#10042
Conversation
WalkthroughThe pull request introduces a new Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 (
|
✅ Deploy Preview for care-ohc ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/components/ui/multi-select.tsx (2)
81-89: Optimize the toggleAll function to avoid unnecessary state updates.When selecting all items, we can avoid the intermediate array creation and reduce state updates.
const toggleAll = () => { if (selectedValues.length === options.length) { handleClear(); } else { - const allValues = options.map((option) => option.value); - setSelectedValues(allValues); - onValueChange(allValues); + const allValues = options.map(({ value }) => value); + setSelectedValues(allValues); + onValueChange(allValues); } };
98-161: Enhance accessibility and performance of the trigger button.Consider these improvements:
- Add aria-label for screen readers
- Memoize click handlers to prevent unnecessary re-renders
<PopoverTrigger asChild> <Button ref={ref} {...props} + aria-label={placeholder} onClick={handleTogglePopover} className={cn(src/components/Facility/FacilityForm.tsx (1)
150-153: Consider type safety improvement in handleFeatureChange.The function could benefit from stronger type safety when converting string values to numbers.
- const handleFeatureChange = (value: string[]) => { - const features = value.map((val) => Number(val)); + const handleFeatureChange = (value: string[]) => { + const features = value.map((val) => { + const num = Number(val); + if (isNaN(num)) { + console.warn(`Invalid feature value: ${val}`); + return 0; + } + return num; + }); form.setValue("features", features); };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/FacilityForm.tsx(4 hunks)src/components/ui/multi-select.tsx(1 hunks)
🔇 Additional comments (4)
src/components/ui/multi-select.tsx (2)
26-39: Well-structured interface definition!The
MultiSelectPropsinterface is well-designed with:
- Clear type definitions
- Optional props for flexibility
- Proper extension of HTML button attributes
41-62: Clean component initialization and state management!Good practices observed:
- Proper use of forwardRef
- Clean state management
- Appropriate prop defaults
src/components/Facility/FacilityForm.tsx (2)
295-315: Clean integration of the MultiSelect component!The implementation correctly:
- Maps facility features to the required format
- Handles value conversion between string and number types
- Provides appropriate placeholder text
521-521: LGTM: Button variant update.The change to "primary" variant aligns with the design system.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
src/components/Form/MultiSelectMenuV2.tsx (1)
Line range hint
16-26: Fix invalid HTML nesting by replacing<p>with<span>elements.Using
<p>elements inside a<span>is invalid HTML, as block-level elements cannot be nested within inline elements. Replace the<p>elements with<span>to ensure correct HTML structure and avoid potential rendering issues.Apply this diff to fix the issue:
export const MultiSelectOptionChip = ({ label, onRemove, }: MultiSelectOptionChipProps) => { return ( - <span className="flex items-center gap-2 rounded-full border-secondary-300 bg-secondary-200 px-3 text-xs text-secondary-700"> - <p className="py-1">{label}</p> - {onRemove && ( - <p + <span className="flex items-center gap-2 rounded-full border-secondary-300 bg-secondary-200 px-3 text-xs text-secondary-700"> + <span className="py-1">{label}</span> + {onRemove && ( + <span className="cursor-pointer rounded-full hover:bg-white" onClick={(e) => { e.stopPropagation(); onRemove(); }} > <CareIcon icon="l-times" className="text-base" /> - </p> + </span> )} </span> ); };
🧹 Nitpick comments (1)
src/components/Form/MultiSelectMenuV2.tsx (1)
Line range hint
1-35: Consider renaming the file to reflect its current content.Since the
MultiSelectMenuV2component has been removed and only utility components (MultiSelectOptionChipanddropdownOptionClassNames) remain, it would be clearer to rename the file to something likeMultiSelectUtils.tsxor move these utilities to a shared components directory. This improves code organization and maintainability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Form/FormFields/SelectFormField.tsx(0 hunks)src/components/Form/MultiSelectMenuV2.tsx(1 hunks)
💤 Files with no reviewable changes (1)
- src/components/Form/FormFields/SelectFormField.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
rithviknishad
added
needs testing
needs review
and removed
Deploy-Failed
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/components/ui/multi-select.tsx (2)
51-51: Localize the default placeholder text usingtfunctionThe
placeholderprop defaults to the hardcoded English string "Select options". To support internationalization, use thetfunction fromreact-i18nextto localize this default text.Apply this diff to fix the issue:
51c51 - placeholder = "Select options", + placeholder = t("select_options") || "Select options",Ensure that the translation key
select_optionsis added to your i18n files.
172-172: Localize the "No results found." messageThe string "No results found." is hardcoded and not localized. To support internationalization, wrap this message with the
tfunction.Apply this diff to fix the issue:
172c172 - <CommandEmpty>No results found.</CommandEmpty> + <CommandEmpty>{t("no_results_found")}</CommandEmpty>Add the key
no_results_foundto your i18n translation files.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/components/ui/multi-select.tsx (4)
51-51: Internationalize hardcoded placeholder for localizationThe
placeholderprop is hardcoded as"Select options", which may not support localization. To enable internationalization, consider wrapping it with the translation functiont.Apply this diff to internationalize the placeholder:
- placeholder = "Select options", + placeholder = t("select_options"),
171-171: Internationalize hardcoded text for localizationThe text
"No results found."is hardcoded in theCommandEmptycomponent. To support localization, wrap it with the translation functiont.Apply this diff to internationalize the text:
- <CommandEmpty>No results found.</CommandEmpty> + <CommandEmpty>{t("no_results_found")}</CommandEmpty>
127-134: Add accessible labels to interactive icons for better accessibilityThe
XCircleicon acts as a button to remove selected options but lacks an accessible label. This can hinder users who rely on screen readers. Consider adding anaria-labelto describe the action.Apply this diff to add an accessible label:
<XCircle className="ml-2 h-4 w-4 cursor-pointer" + aria-label="Remove option" onClick={(event) => { event.stopPropagation(); toggleOption(value); }} />
139-145: Add accessible labels to interactive icons for better accessibilityThe
XIconused to clear all selections lacks an accessible label. Adding anaria-labelwill improve accessibility for screen reader users.Apply this diff to add an accessible label:
<XIcon className="h-4 mx-2 cursor-pointer text-black" + aria-label="Clear all selections" onClick={(event) => { event.stopPropagation(); handleClear(); }} />src/components/Facility/FacilityForm.tsx (2)
298-298: Internationalize hardcoded labels for localizationThe
FormLabeltext"Features"is hardcoded. To support localization, wrap it with the translation functiont.Apply this diff to internationalize the label:
<FormLabel>Features</FormLabel> + <FormLabel>{t("features")}</FormLabel>
308-308: Internationalize hardcoded placeholder for localizationThe
placeholderprop is hardcoded as"Select facility features". Consider using thetfunction to support multiple languages.Apply this diff to internationalize the placeholder:
placeholder="Select facility features" + placeholder={t("select_facility_features")}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Facility/FacilityForm.tsx(4 hunks)src/components/ui/multi-select.tsx(1 hunks)
🔇 Additional comments (1)
src/components/ui/multi-select.tsx (1)
155-158: Avoid duplicateidattributes to ensure unique identifiersAs previously mentioned, the
idattribute"dropdown-toggle"is duplicated. Removing the duplicateidwill prevent potential conflicts.Apply this diff to remove the duplicate
idattribute:<ChevronDown - id="dropdown-toggle" className="h-4 mx-2 cursor-pointer text-black" />
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/components/ui/multi-select.tsx (3)
57-58:⚠️ Potential issueEnsure
selectedValuesis always an array to prevent runtime errorsThe
selectedValuesstate is initialized withvalue, which could beundefined. This could lead to runtime errors since the component expects it to be an array.Apply this diff to fix the issue:
- const [selectedValues, setSelectedValues] = React.useState<string[]>(value); + const [selectedValues, setSelectedValues] = React.useState<string[]>(value || []);
59-63:⚠️ Potential issueEnsure effect hook handles undefined values safely
The effect hook updates
selectedValueswithvaluewithout checking for undefined, which could cause runtime errors.Apply this diff to fix the issue:
React.useEffect(() => { - setSelectedValues(value); + setSelectedValues(value || []); }, [value]);
145-148:⚠️ Potential issueAvoid duplicate
idattributes to ensure unique identifiersThe
idattribute"dropdown-toggle"is used multiple times within the component. In HTML,idattributes must be unique within the DOM.Apply this diff to fix the issue:
<ChevronDown - id="dropdown-toggle" className="h-4 mx-2 cursor-pointer text-black" />Also applies to: 154-157
🧹 Nitpick comments (2)
src/components/ui/multi-select.tsx (2)
26-39: Enhance type safety with TypeScript improvements.Consider these TypeScript enhancements:
- Add
readonlymodifiers to props to ensure immutability- Type the
iconprop properly instead of using a string typeinterface MultiSelectProps extends React.ButtonHTMLAttributes<HTMLButtonElement> { - options: { + readonly options: { label: string; value: string; - icon?: IconName; + icon?: IconName | undefined; }[]; - onValueChange: (value: string[]) => void; - value: string[]; - placeholder: string; + readonly onValueChange: (value: readonly string[]) => void; + readonly value: readonly string[]; + readonly placeholder: string; modalPopover?: boolean; asChild?: boolean; className?: string; }
65-90: Optimize handlers for better performance.Consider these performance improvements:
- Memoize the
allValuesarray intoggleAll- Use Set for faster lookups in
toggleOption+ const allValues = React.useMemo( + () => options.map((option) => option.value), + [options] + ); const toggleOption = (option: string) => { + const valueSet = new Set(selectedValues); + if (valueSet.has(option)) { + valueSet.delete(option); + } else { + valueSet.add(option); + } + const newSelectedValues = Array.from(valueSet); - const newSelectedValues = selectedValues.includes(option) - ? selectedValues.filter((value) => value !== option) - : [...selectedValues, option]; setSelectedValues(newSelectedValues); onValueChange(newSelectedValues); }; const toggleAll = () => { if (selectedValues.length === options.length) { handleClear(); } else { - const allValues = options.map((option) => option.value); setSelectedValues(allValues); onValueChange(allValues); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
public/locale/en.json(1 hunks)src/components/Facility/FacilityForm.tsx(4 hunks)src/components/ui/multi-select.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/Facility/FacilityForm.tsx
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
public/locale/en.json (1)
1857-1857: LGTM! Translation key added correctly.The new translation key follows the existing naming pattern and provides a clear, user-friendly message.
|
LGTM |
nihal467
added
tested
reviewed
|
@AdityaJ2305 Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
MultiOptionSelectshadcn component #10041@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
MultiSelectcomponent for enhanced multi-option selection.Bug Fixes
Style
Chores
MultiSelectFormFieldandMultiSelectMenuV2components.