Fix: Facility Organization Dropdown #10104
Conversation
WalkthroughThe pull request enhances the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
✅ 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: 1
🧹 Nitpick comments (1)
src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx (1)
115-126: Consider adding ARIA attributes for better accessibility.The new organization display looks great with the Building icon and clear visual hierarchy. Consider these accessibility improvements:
- <div className="flex items-center gap-3 rounded-md border border-sky-100 bg-sky-50/50 p-2.5"> + <div + className="flex items-center gap-3 rounded-md border border-sky-100 bg-sky-50/50 p-2.5" + role="status" + aria-label={`Selected organization: ${selectedOrganization.name}`} + > <Building className="h-4 w-4 text-sky-600 flex-shrink-0" aria-hidden="true" />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx(2 hunks)
🔇 Additional comments (2)
src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx (2)
2-2: LGTM! Good choice of icon library.The addition of the Building icon from lucide-react aligns with the PR objectives and follows good practices.
172-176: LGTM! Well-implemented dynamic placeholder with i18n support.The placeholder text implementation provides clear context and properly supports internationalization.
| <div key={org.id} className="flex items-center gap-1.5"> | ||
| <div className="flex items-center gap-1.5 flex-1"> | ||
| {index > 0 && ( | ||
| <CareIcon | ||
| icon="l-arrow-right" | ||
| className="h-3.5 w-3.5 text-gray-400 flex-shrink-0" | ||
| /> | ||
| )} | ||
| <div | ||
| className="flex items-center justify-between flex-1 rounded-md border border-gray-200 bg-white px-2.5 py-1.5 text-sm shadow-sm cursor-pointer hover:bg-gray-50" | ||
| onClick={() => handleEdit(index)} | ||
| > | ||
| <span className="truncate">{org.name}</span> | ||
| <CareIcon | ||
| icon="l-angle-down" | ||
| className="h-3.5 w-3.5 ml-2 flex-shrink-0 text-gray-400" | ||
| /> | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance keyboard accessibility for interactive elements.
The hierarchical display looks clean, but needs accessibility improvements for keyboard users:
<div
- className="flex items-center justify-between flex-1 rounded-md border border-gray-200 bg-white px-2.5 py-1.5 text-sm shadow-sm cursor-pointer hover:bg-gray-50"
+ className="flex items-center justify-between flex-1 rounded-md border border-gray-200 bg-white px-2.5 py-1.5 text-sm shadow-sm cursor-pointer hover:bg-gray-50 focus:outline-none focus:ring-2 focus:ring-sky-500 focus:ring-offset-2"
+ role="button"
+ tabIndex={0}
onClick={() => handleEdit(index)}
+ onKeyDown={(e) => e.key === 'Enter' && handleEdit(index)}
+ aria-label={`Edit ${org.name} level`}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <div key={org.id} className="flex items-center gap-1.5"> | |
| <div className="flex items-center gap-1.5 flex-1"> | |
| {index > 0 && ( | |
| <CareIcon | |
| icon="l-arrow-right" | |
| className="h-3.5 w-3.5 text-gray-400 flex-shrink-0" | |
| /> | |
| )} | |
| <div | |
| className="flex items-center justify-between flex-1 rounded-md border border-gray-200 bg-white px-2.5 py-1.5 text-sm shadow-sm cursor-pointer hover:bg-gray-50" | |
| onClick={() => handleEdit(index)} | |
| > | |
| <span className="truncate">{org.name}</span> | |
| <CareIcon | |
| icon="l-angle-down" | |
| className="h-3.5 w-3.5 ml-2 flex-shrink-0 text-gray-400" | |
| /> | |
| </div> | |
| </div> | |
| </div> | |
| <div key={org.id} className="flex items-center gap-1.5"> | |
| <div className="flex items-center gap-1.5 flex-1"> | |
| {index > 0 && ( | |
| <CareIcon | |
| icon="l-arrow-right" | |
| className="h-3.5 w-3.5 text-gray-400 flex-shrink-0" | |
| /> | |
| )} | |
| <div | |
| className="flex items-center justify-between flex-1 rounded-md border border-gray-200 bg-white px-2.5 py-1.5 text-sm shadow-sm cursor-pointer hover:bg-gray-50 focus:outline-none focus:ring-2 focus:ring-sky-500 focus:ring-offset-2" | |
| role="button" | |
| tabIndex={0} | |
| onClick={() => handleEdit(index)} | |
| onKeyDown={(e) => e.key === 'Enter' && handleEdit(index)} | |
| aria-label={`Edit ${org.name} level`} | |
| > | |
| <span className="truncate">{org.name}</span> | |
| <CareIcon | |
| icon="l-angle-down" | |
| className="h-3.5 w-3.5 ml-2 flex-shrink-0 text-gray-400" | |
| /> | |
| </div> | |
| </div> | |
| </div> |
|
When you click on the dropdown that already has a value selected, the expected behavior is for the dropdown options to be displayed. Instead, the current behavior is that it closes the entire dropdown and reset it to its initial state, as if no value was previously selected. @Jeffrin2005 https://www.loom.com/share/8cb97e50e22a45f98702f155c1ae9685?sid=6d2cc770-b044-409c-9801-2e665845b678 |
v23.mp4Any Changes required ? |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/components/ui/autocomplete.tsx (1)
Line range hint
33-47: Consider adding controlled open state prop.To provide more flexibility and better control over the dropdown's behavior, consider making the open state controllable from the parent component.
Add an optional prop for controlled open state:
interface AutocompleteProps { options: AutoCompleteOption[]; value: string; onChange: (value: string) => void; onSearch?: (value: string) => void; placeholder?: string; noOptionsMessage?: string; disabled?: boolean; align?: "start" | "center" | "end"; popoverClassName?: string; "data-cy"?: string; + open?: boolean; + onOpenChange?: (open: boolean) => void; }This would allow parent components to have full control over the dropdown's open state when needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/autocomplete.tsx(2 hunks)src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx
🔇 Additional comments (2)
src/components/ui/autocomplete.tsx (2)
52-52: LGTM! Good optimization.The introduction of
selectedOptionimproves code readability and reduces repeated lookups.
67-73: LGTM! Clean implementation.The label display logic is well-structured with good separation of styling and content.
| onChange(currentValue); | ||
| setOpen(false); | ||
| }} |
There was a problem hiding this comment.
Optimize onSelect handler to prevent unnecessary updates.
The current implementation doesn't check if the selected value is different from the current value, which could lead to unnecessary re-renders and might contribute to the reported dropdown behavior issues.
Apply this optimization:
- onChange(currentValue);
- setOpen(false);
+ if (currentValue !== value) {
+ onChange(currentValue);
+ setOpen(false);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onChange(currentValue); | |
| setOpen(false); | |
| }} | |
| if (currentValue !== value) { | |
| onChange(currentValue); | |
| setOpen(false); | |
| } |
src/components/ui/autocomplete.tsx
Outdated
| title={selectedOption?.label} | ||
| variant="outline" | ||
| role="combobox" | ||
| aria-expanded={open} | ||
| className="w-full justify-between" | ||
| disabled={disabled} | ||
| data-cy={dataCy} | ||
| onClick={() => setOpen(true)} |
There was a problem hiding this comment.
Fix dropdown toggle behavior.
The current onClick implementation doesn't properly handle the toggle case, which explains the reported issue where "dropdown closes when clicked with a value selected". The dropdown should toggle between open/closed states.
Apply this fix:
- onClick={() => setOpen(true)}
+ onClick={() => setOpen(!open)}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| title={selectedOption?.label} | |
| variant="outline" | |
| role="combobox" | |
| aria-expanded={open} | |
| className="w-full justify-between" | |
| disabled={disabled} | |
| data-cy={dataCy} | |
| onClick={() => setOpen(true)} | |
| title={selectedOption?.label} | |
| variant="outline" | |
| role="combobox" | |
| aria-expanded={open} | |
| className="w-full justify-between" | |
| disabled={disabled} | |
| data-cy={dataCy} | |
| onClick={() => setOpen(!open)} |
src/components/ui/autocomplete.tsx
Outdated
| <span | ||
| className={cn( | ||
| "truncate", | ||
| !selectedOption && "text-muted-foreground", |
There was a problem hiding this comment.
are you sure this is a valid tailwind class?
| {selectedOrganization.has_children && ( | ||
| <Badge variant="outline">Has Sub-departments</Badge> | ||
| <p className="text-xs text-sky-600"> | ||
| {t("Has Sub Departments")} |
There was a problem hiding this comment.
refer translations guidelines in README. this is not how i18n should be done.
| if (level < selectedLevels.length) { | ||
| return selectedLevels[level].name; | ||
| } | ||
| return level === 0 ? t("select_department") : t("select sub department"); |
There was a problem hiding this comment.
please check all translations
rithviknishad
added
invalid
|
@rithviknishad Updated all transitions. |
src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx
Show resolved
Hide resolved
| <div | ||
| className="cursor-pointer p-1 hover:bg-gray-100 rounded-sm opacity-0 group-hover:opacity-100 transition-opacity" | ||
| onClick={() => { | ||
| const newLevels = selectedLevels.slice(0, level); |
There was a problem hiding this comment.
This logic is what should be in a function called handleEdit 👍
Separate out the concerns, code readability is important (check GovtOrganizationSelector for example for similar logic).
Jacobjeevan
added
changes required
and removed
invalid
| } | ||
| }; | ||
|
|
||
| const renderOrganizationLevel = (level: number) => { |
There was a problem hiding this comment.
Intermediate level options don't show up.
CurrentLevelOrganizations query is explicitly tied to the very last entry in selectedLevels, so this approach won't work when making existing fields editable.
There was a problem hiding this comment.
@Jacobjeevan Removed search option for intermediate level , only for last level
v23.mp4
There was a problem hiding this comment.
🤔 This isn't really solving the issue though - why even add autocomplete if we are just disabling it? Can just keep the existing code as is.
No, we do want to make the intermediate levels editable. You can look at GovtOrganizationSelector or OrganizationFilter on hints on how to accomplish that.
src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx (1)
138-144: 🛠️ Refactor suggestionEnhance keyboard accessibility for interactive elements.
The dropdown trigger needs proper keyboard accessibility support.
Apply these changes:
<div className="cursor-pointer p-1 hover:bg-gray-100 rounded-sm opacity-0 group-hover:opacity-100 transition-opacity" + role="button" + tabIndex={0} onClick={() => handleEdit(level)} + onKeyDown={(e) => e.key === 'Enter' && handleEdit(level)} + aria-label={t("edit_organization_level", { level: level + 1 })} > <CareIcon icon="l-pen" className="h-4 w-4 text-gray-500" /> </div>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/ui/autocomplete.tsx(2 hunks)src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/components/ui/autocomplete.tsx
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: OSSAR-Scan
- GitHub Check: cypress-run (1)
🔇 Additional comments (1)
src/pages/FacilityOrganization/components/FacilityOrganizationSelector.tsx (1)
2-2: LGTM: Building icon import aligns with requirements.The addition of the Building icon import supports the UI enhancement mentioned in the PR objectives.
|
|
||
| if (newLevels.length > 0) { | ||
| const lastOrg = newLevels[newLevels.length - 1]; | ||
| setSelectedOrganization(lastOrg); | ||
| onChange(lastOrg.id); | ||
| } else { | ||
| setSelectedOrganization(null); | ||
| onChange(""); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Rename function to reflect its actual purpose and fix dropdown state reset.
- The function name
handleEditdoesn't accurately describe its purpose of resetting/updating the selection state. - The current implementation resets the dropdown state, causing the issue reported in PR comments where clicking a dropdown with a selected value closes it instead of showing options.
Consider these changes:
- Rename the function:
- const handleEdit = (level: number) => {
+ const handleResetSelection = (level: number) => {- Update all references:
- onClick={() => handleEdit(level)}
+ onClick={() => handleResetSelection(level)}- Fix the dropdown state reset by conditionally updating the state:
const handleResetSelection = (level: number) => {
// Only reset if clicking on a different level than current
if (level !== selectedLevels.length - 1) {
const newLevels = selectedLevels.slice(0, level);
setSelectedLevels(newLevels);
if (newLevels.length > 0) {
const lastOrg = newLevels[newLevels.length - 1];
setSelectedOrganization(lastOrg);
onChange(lastOrg.id);
} else {
setSelectedOrganization(null);
onChange("");
}
}
};| if (level < selectedLevels.length) { | ||
| return selectedLevels[level].name; | ||
| } | ||
| return level === 0 ? t("select_department") : t("select_sub_department"); |
There was a problem hiding this comment.
Follow project's translation guidelines.
Direct string literals in t() function don't follow the project's translation guidelines as mentioned in previous reviews.
- Move all translations to the locale files:
- return level === 0 ? t("select_department") : t("select_sub_department");
+ return level === 0 ? t("facility.organization.select_department") : t("facility.organization.select_sub_department");
- {t("has_sub_departments")}
+ {t("facility.organization.has_sub_departments")}- Update the locale files to include these keys in a structured manner:
{
"facility": {
"organization": {
"select_department": "Select Department",
"select_sub_department": "Select Sub-department",
"has_sub_departments": "Has sub-departments"
}
}
}Also applies to: 165-165
| const renderOrganizationLevel = (level: number) => { | ||
| const orgList = | ||
| level === 0 | ||
| ? getAllOrganizations?.results | ||
| : currentLevelOrganizations?.results; | ||
| const getDropdownLabel = () => { | ||
| if (level < selectedLevels.length) { | ||
| return selectedLevels[level].name; | ||
| } | ||
| return level === 0 ? t("select_department") : t("select_sub_department"); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="group flex items-center gap-1.5"> | ||
| {level > 0 && ( | ||
| <CareIcon | ||
| icon="l-arrow-right" | ||
| className="h-3.5 w-3.5 text-gray-400 flex-shrink-0" | ||
| /> | ||
| )} | ||
| <div className="flex-1 flex items-center gap-2"> | ||
| <div className="flex-1"> | ||
| {level < selectedLevels.length && | ||
| selectedLevels[level].has_children ? ( | ||
| <div className="px-3 py-2 text-sm border rounded-md bg-white"> | ||
| {selectedLevels[level].name} | ||
| </div> | ||
| ) : ( | ||
| <Autocomplete | ||
| value={selectedLevels[level]?.id} | ||
| options={getOrganizationOptions(orgList)} | ||
| onChange={(value) => handleLevelChange(value, level)} | ||
| placeholder={getDropdownLabel()} | ||
| /> | ||
| )} | ||
| </div> | ||
| {level < selectedLevels.length && | ||
| selectedLevels[level].has_children && ( | ||
| <div | ||
| className="cursor-pointer p-1 hover:bg-gray-100 rounded-sm opacity-0 group-hover:opacity-100 transition-opacity" | ||
| onClick={() => handleEdit(level)} | ||
| > | ||
| <CareIcon icon="l-pen" className="h-4 w-4 text-gray-500" /> | ||
| </div> | ||
| )} | ||
| </div> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
Update icon to match requirements and fix intermediate level options.
- The PR objectives mention replacing the edit pencil with a dropdown arrow, but the code still uses the pencil icon.
- The intermediate level options issue might still persist due to the current query logic.
Apply these changes:
- Replace pencil icon with dropdown arrow:
- <CareIcon icon="l-pen" className="h-4 w-4 text-gray-500" />
+ <CareIcon icon="l-chevron-down" className="h-4 w-4 text-gray-500" />- Fix intermediate level options by updating the query logic:
- const { data: currentLevelOrganizations } = useQuery<{
+ const { data: currentLevelOrganizations, refetch: refetchOrganizations } = useQuery<{
results: FacilityOrganization[];
}>({
queryKey: [
"organizations-current",
- selectedLevels[selectedLevels.length - 1]?.id,
+ selectedLevels[level]?.id, // Use current level's ID
],
queryFn: query(routes.facilityOrganization.list, {
pathParams: { facilityId },
queryParams: {
- parent: selectedLevels[selectedLevels.length - 1]?.id,
+ parent: selectedLevels[level]?.id, // Use current level's ID
},
}),
- enabled: selectedLevels.length > 0,
+ enabled: level < selectedLevels.length,
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const renderOrganizationLevel = (level: number) => { | |
| const orgList = | |
| level === 0 | |
| ? getAllOrganizations?.results | |
| : currentLevelOrganizations?.results; | |
| const getDropdownLabel = () => { | |
| if (level < selectedLevels.length) { | |
| return selectedLevels[level].name; | |
| } | |
| return level === 0 ? t("select_department") : t("select_sub_department"); | |
| }; | |
| return ( | |
| <div className="group flex items-center gap-1.5"> | |
| {level > 0 && ( | |
| <CareIcon | |
| icon="l-arrow-right" | |
| className="h-3.5 w-3.5 text-gray-400 flex-shrink-0" | |
| /> | |
| )} | |
| <div className="flex-1 flex items-center gap-2"> | |
| <div className="flex-1"> | |
| {level < selectedLevels.length && | |
| selectedLevels[level].has_children ? ( | |
| <div className="px-3 py-2 text-sm border rounded-md bg-white"> | |
| {selectedLevels[level].name} | |
| </div> | |
| ) : ( | |
| <Autocomplete | |
| value={selectedLevels[level]?.id} | |
| options={getOrganizationOptions(orgList)} | |
| onChange={(value) => handleLevelChange(value, level)} | |
| placeholder={getDropdownLabel()} | |
| /> | |
| )} | |
| </div> | |
| {level < selectedLevels.length && | |
| selectedLevels[level].has_children && ( | |
| <div | |
| className="cursor-pointer p-1 hover:bg-gray-100 rounded-sm opacity-0 group-hover:opacity-100 transition-opacity" | |
| onClick={() => handleEdit(level)} | |
| > | |
| <CareIcon icon="l-pen" className="h-4 w-4 text-gray-500" /> | |
| </div> | |
| )} | |
| </div> | |
| </div> | |
| ); | |
| }; | |
| const renderOrganizationLevel = (level: number) => { | |
| const orgList = | |
| level === 0 | |
| ? getAllOrganizations?.results | |
| : currentLevelOrganizations?.results; | |
| const getDropdownLabel = () => { | |
| if (level < selectedLevels.length) { | |
| return selectedLevels[level].name; | |
| } | |
| return level === 0 ? t("select_department") : t("select_sub_department"); | |
| }; | |
| return ( | |
| <div className="group flex items-center gap-1.5"> | |
| {level > 0 && ( | |
| <CareIcon | |
| icon="l-arrow-right" | |
| className="h-3.5 w-3.5 text-gray-400 flex-shrink-0" | |
| /> | |
| )} | |
| <div className="flex-1 flex items-center gap-2"> | |
| <div className="flex-1"> | |
| {level < selectedLevels.length && | |
| selectedLevels[level].has_children ? ( | |
| <div className="px-3 py-2 text-sm border rounded-md bg-white"> | |
| {selectedLevels[level].name} | |
| </div> | |
| ) : ( | |
| <Autocomplete | |
| value={selectedLevels[level]?.id} | |
| options={getOrganizationOptions(orgList)} | |
| onChange={(value) => handleLevelChange(value, level)} | |
| placeholder={getDropdownLabel()} | |
| /> | |
| )} | |
| </div> | |
| {level < selectedLevels.length && | |
| selectedLevels[level].has_children && ( | |
| <div | |
| className="cursor-pointer p-1 hover:bg-gray-100 rounded-sm opacity-0 group-hover:opacity-100 transition-opacity" | |
| onClick={() => handleEdit(level)} | |
| > | |
| <CareIcon icon="l-chevron-down" className="h-4 w-4 text-gray-500" /> | |
| </div> | |
| )} | |
| </div> | |
| </div> | |
| ); | |
| }; |
|
👋 Hi, @Jeffrin2005, |
Proposed Changes
Fixes Enhance Facility Organization Dropdown Experience #10060
Changed from edit pencil icon to dropdown arrow
Added building icon only for the main selected organization with blue color
v23.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Summary by CodeRabbit
UI/UX Improvements
Buildingicon to organization selection.Localization Enhancements