Feat: Update the design and shortcut key for SearchByMultipleOptions #9851
Conversation
|
Warning Rate limit exceeded@AdityaJ2305 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (4)
WalkthroughThis pull request introduces changes to several components to enhance search functionality and user experience. Key modifications include the removal of 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. |
|
@AdityaJ2305 I think you can make use of |
|
Fixed: #9867 (comment) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/pages/Organization/OrganizationPatients.tsx (1)
41-52: Consider improving phone number validation.The phone number validation logic could be more robust:
- The magic number
13should be extracted as a constant for better maintainability.- The validation should consider minimum length requirements for different phone number formats.
Apply this diff to improve the validation:
+const MIN_PHONE_LENGTH = 13; + const handleSearch = useCallback((key: string, value: string) => { const searchParams = { name: key === "name" ? value : "", phone_number: key === "phone_number" - ? value.length >= 13 || value === "" + ? value.length >= MIN_PHONE_LENGTH || value === "" ? value : undefined : undefined, }; updateQuery(searchParams); }, []);src/components/Common/SearchByMultipleFields.tsx (2)
191-214: Reduce code duplication in keyboard shortcut hints.The keyboard shortcut hint UI is duplicated between the phone number input and text input sections. Consider extracting it into a reusable component.
Create a new component for the keyboard hints:
const KeyboardShortcutHint: React.FC<{ isOpen: boolean }> = ({ isOpen }) => { if (isOpen) { return ( <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> ESC </span> ); } return ( <span> {isAppleDevice ? ( <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> ⌘K </span> ) : ( <div className="flex gap-1 font-medium"> <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> Ctrl </span> <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> K </span> </div> )} </span> ); };Then use it in both places:
<div className="absolute top-1/2 right-2 transform -translate-y-1/2 flex items-center space-x-2 text-xs text-gray-500"> - {open ? ( - <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> - ESC - </span> - ) : ( - <span> - {isAppleDevice ? ( - <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> - ⌘K - </span> - ) : ( - <div className="flex gap-1 font-medium"> - <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> - Ctrl - </span> - <span className="border border-gray-300 rounded px-1 py-0.5 bg-white text-gray-500"> - K - </span> - </div> - )} - </span> - )} + <KeyboardShortcutHint isOpen={open} /> </div>Also applies to: 226-249
130-140: Enhance keyboard navigation robustness.The keyboard navigation logic could be improved in two areas:
- Add visual feedback when no item is focused (focusedIndex === -1)
- Make the Enter key handler more defensive
Apply these improvements:
} else if (e.key === "Enter" && focusedIndex !== -1) { + if (focusedIndex >= unselectedOptions.length) { + return; + } const selectedOptionIndex = options.findIndex( (option) => option.key === unselectedOptions[focusedIndex].key, ); + if (selectedOptionIndex === -1) { + return; + } handleOptionChange(selectedOptionIndex); }Also, consider adding a CSS class to indicate when no item is focused:
className={cn( "flex items-center p-2 rounded-md cursor-pointer", { "bg-gray-100": focusedIndex === index, + "bg-gray-50": focusedIndex === -1, "hover:bg-secondary-100": true, }, )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Common/SearchByMultipleFields.tsx(5 hunks)src/pages/Encounters/EncounterList.tsx(2 hunks)src/pages/Organization/OrganizationPatients.tsx(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Encounters/EncounterList.tsx
[error] 165-175: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
src/pages/Organization/OrganizationPatients.tsx
[error] 55-61: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (1)
src/components/Common/SearchByMultipleFields.tsx (1)
118-126: LGTM! Keyboard shortcut implementation follows best practices.The change from "/" to "Cmd/Ctrl + K" aligns with modern web application patterns and provides a more intuitive search experience. The implementation correctly handles both Mac and Windows/Linux modifier keys.
|
👋 Hi, @AdityaJ2305, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
AdityaJ2305
force-pushed
the
serachinput_design_update
branch
from
91e9334 to
3cfb24a
Compare
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/pages/Facility/FacilitiesPage.tsx (1)
73-73: Consider implementing keyboard shortcuts using useKeyboardShortcut hookWhile setting initialOptionIndex is good for UX, consider implementing keyboard shortcuts using the useKeyboardShortcut hook as suggested in the PR comments. This would provide better accessibility and user experience.
+import useKeyboardShortcut from "use-keyboard-shortcut"; + export function FacilitiesPage() { + const handleSearchFocus = useCallback(() => { + document.getElementById("facility-search")?.focus(); + }, []); + + useKeyboardShortcut(["shift", "f"], handleSearchFocus);src/pages/Organization/components/GovtOrganizationSelector.tsx (1)
Line range hint
15-28: LGTM! Consider adding JSDoc commentsThe renaming to GovtOrganizationSelector better reflects the component's purpose. Consider adding JSDoc comments to document the component's specific use for government organizations and the props interface.
+/** + * Props for the GovtOrganizationSelector component + * @interface GovtOrganizationSelectorProps + */ interface GovtOrganizationSelectorProps { value?: string; onChange: (value: string) => void; required?: boolean; authToken?: string; } +/** + * A component for selecting government organizations in a hierarchical manner + * @component + */ export default function GovtOrganizationSelector( props: GovtOrganizationSelectorProps, ) {src/pages/PublicAppointments/PatientRegistration.tsx (1)
Line range hint
412-419: Add error handling for the geo_organization field.While the component implementation is correct, consider adding error handling to display validation errors for the geo_organization field, similar to other form fields in the component.
<GovtOrganizationSelector required authToken={tokenData.token} onChange={(value) => { field.onChange(value); }} /> +<FormMessage />src/components/Facility/FacilityCreate.tsx (1)
Line range hint
333-337: Enhance the GovtOrganizationSelector implementation.Consider these improvements:
- Add error handling to display validation errors
- Use FormField wrapper for consistency with other form fields
-<GovtOrganizationSelector - required={true} - onChange={(value) => form.setValue("geo_organization", value)} -/> +<FormField + control={form.control} + name="geo_organization" + render={({ field }) => ( + <FormItem> + <FormControl> + <GovtOrganizationSelector + required={true} + onChange={field.onChange} + value={field.value} + /> + </FormControl> + <FormMessage /> + </FormItem> + )} +/>src/components/Patient/PatientRegistration.tsx (1)
Line range hint
722-731: Remove unnecessary Fragment wrapper.The Fragment wrapper (
<>...</>) is not needed here as there's only one child element.-<> <GovtOrganizationSelector required={true} onChange={(value) => setForm((f) => ({ ...f, geo_organization: value, })) } /> -</>src/pages/Organization/OrganizationPatients.tsx (1)
56-64: Remove debug console.log statement.The function implementation is correct, but it contains a debug console.log statement that should be removed before merging.
Apply this diff to remove the debug statement:
const handleSearch = useCallback((key: string, value: string) => { const searchParams = { name: key === "name" ? value : "", phone_number: key === "phone_number" ? (value.length == 3 ? "" : value) : "", }; - console.log(searchParams.phone_number); updateQuery(searchParams); }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/components/Common/SearchByMultipleFields.tsx(7 hunks)src/components/Facility/CreateFacilityForm.tsx(2 hunks)src/components/Facility/FacilityCreate.tsx(2 hunks)src/components/Patient/PatientIndex.tsx(2 hunks)src/components/Patient/PatientRegistration.tsx(2 hunks)src/components/Users/CreateUserForm.tsx(2 hunks)src/pages/Encounters/EncounterList.tsx(2 hunks)src/pages/Facility/FacilitiesPage.tsx(1 hunks)src/pages/Organization/OrganizationPatients.tsx(3 hunks)src/pages/Organization/components/GovtOrganizationSelector.tsx(2 hunks)src/pages/PublicAppointments/PatientRegistration.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Organization/OrganizationPatients.tsx
[error] 67-73: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
src/pages/Encounters/EncounterList.tsx
[error] 165-175: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (12)
src/components/Patient/PatientIndex.tsx (2)
94-94: Review phone number clearing logicThe current logic clears the phone number when length is exactly 3, which might disrupt user input. Consider keeping the previous logic or clarify the reasoning behind this specific condition.
Could you explain why the phone number should be cleared specifically when the length is 3?
148-148: LGTM! Good use of keyboard shortcutsThe addition of initialOptionIndex is good for UX, and the component already implements keyboard shortcuts properly using useKeyboardShortcut hook.
src/components/Facility/CreateFacilityForm.tsx (1)
Line range hint
421-425: LGTM! Consistent component usageThe update to use GovtOrganizationSelector is consistent with the component rename and maintains all necessary props and validation.
src/components/Users/CreateUserForm.tsx (1)
Line range hint
468-472: Clean implementation of GovtOrganizationSelector!The component is properly integrated with the form's validation schema and control flow. The required prop is correctly passed, and the onChange handler is properly wired to update the form state.
src/pages/Organization/OrganizationPatients.tsx (3)
31-38: LGTM!The hook's return value is correctly destructured to include
clearSearch, and thecacheBlacklistis appropriately configured for the search fields.
41-54: LGTM!The search options are well-structured with appropriate types and placeholders for each field.
66-74: Remove redundant block statement.The function contains an unnecessary block statement that can be safely removed.
Apply this diff to simplify the code:
const handleFieldChange = () => { - { - updateQuery({ - name: undefined, - phone_number: undefined, - }); - clearSearch.value = true; - } + updateQuery({ + name: undefined, + phone_number: undefined, + }); + clearSearch.value = true; };🧰 Tools
🪛 Biome (1.9.4)
[error] 67-73: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
src/components/Common/SearchByMultipleFields.tsx (4)
31-31: LGTM! Good improvements to the component's API.Making
initialOptionIndexrequired and adding device-specific keyboard shortcut support improves the component's reliability and user experience.Also applies to: 45-45
104-113: LGTM! Improved focus management.The memoized
unselectedOptionsand focus management effect enhance the component's performance and accessibility.
117-125: Document the keyboard shortcut change.The keyboard shortcut has been changed from "/" to "⌘K/Ctrl+K". While this aligns better with platform conventions, it's a breaking change that should be documented.
Please ensure this change is:
- Documented in the changelog
- Communicated to users
- Added to the component's documentation
Would you like me to help draft the documentation for this change?
183-218: LGTM! Enhanced UI with clear keyboard shortcut hints.The UI now clearly shows keyboard shortcuts with device-specific indicators (⌘K for macOS, Ctrl+K for others). The implementation is clean and well-structured.
Also applies to: 222-253
src/pages/Encounters/EncounterList.tsx (1)
164-176: Remove redundant block statement.The function contains an unnecessary block statement, similar to the one in OrganizationPatients.tsx.
Apply this diff to simplify the code:
const handleFieldChange = () => { - { - updateQuery({ - status, - encounter_class: encounterClass, - priority, - name: undefined, - encounter_id: undefined, - external_identifier: undefined, - }); - clearSearch.value = true; - } + updateQuery({ + status, + encounter_class: encounterClass, + priority, + name: undefined, + encounter_id: undefined, + external_identifier: undefined, + }); + clearSearch.value = true; };🧰 Tools
🪛 Biome (1.9.4)
[error] 165-175: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/pages/Organization/OrganizationPatients.tsx (3)
31-38: Document the significance of the limit value.Consider adding a comment explaining why the limit is set to 14 items per page, as this seems like a non-standard value.
41-54: Consider using a type-safe approach for search options.Instead of type assertions, consider defining an enum or union type for the search field types.
type SearchFieldType = 'text' | 'phone'; const searchOptions = [ { key: "name", type: "text" satisfies SearchFieldType, placeholder: "Search by name", value: qParams.name || "", }, // ... ];
107-113: Simplify complex conditions.Consider these improvements for better readability:
- Extract the initialOptionIndex calculation to a named variable
- Simplify the clearSearch condition
const activeSearchIndex = searchOptions.findIndex(option => option.value !== ""); const hasActiveSearch = qParams.name || qParams.phone_number; <SearchByMultipleFields id="patient-search" options={searchOptions} initialOptionIndex={activeSearchIndex} onSearch={handleSearch} clearSearch={{ value: !hasActiveSearch }} onFieldChange={handleFieldChange} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/pages/Organization/OrganizationPatients.tsx(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/pages/Organization/OrganizationPatients.tsx
[error] 66-72: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.
(lint/complexity/noUselessLoneBlockStatements)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (2)
src/pages/Organization/OrganizationPatients.tsx (2)
56-63: Document phone number validation logic.The condition
value.length == 3for clearing phone number search is not intuitive. Consider adding a comment explaining this validation rule.
65-73: Remove redundant block statement.The function contains an unnecessary block statement that can be safely removed.
Apply this diff to simplify the code:
const handleFieldChange = () => { - { - updateQuery({ - name: undefined, - phone_number: undefined, - }); - clearSearch.value = true; - } + updateQuery({ + name: undefined, + phone_number: undefined, + }); + clearSearch.value = true; };🧰 Tools
🪛 Biome (1.9.4)
[error] 66-72: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
Safe fix: Remove redundant block.(lint/complexity/noUselessLoneBlockStatements)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/components/Common/SearchByMultipleFields.tsx (4)
117-125: Document the new keyboard shortcutThe change from "/" to "Cmd/Ctrl + K" is a good improvement, but should be documented for users.
Consider adding a comment explaining the shortcut change:
+// Using Cmd/Ctrl + K as it's a more standard shortcut for search functionality if (e.key === "k" && (e.metaKey || e.ctrlKey)) {
155-159: Improve phone number validation logicThe phone number validation logic could be more robust. Consider extracting the validation to a separate function.
+const isValidPhoneNumber = (value: string) => value.length === 13 || value.length === 3; + if ( - selectedOption.key === "phone_number" - ? searchValue.length == 13 || searchValue.length == 3 - : selectedOption.value !== searchValue + selectedOption.key === "phone_number" + ? isValidPhoneNumber(searchValue) + : selectedOption.value !== searchValue ) {
397-409: Add data-testid for clear search buttonThe clear search button is a good addition, but should include a data-testid for testing.
<Button variant="ghost" size="sm" + data-testid={`${id}__clear-search`} className="w-full justify-start text-muted-foreground" onClick={() => { setSearchValue(""); }} >
281-292: Enhance accessibility with ARIA labelsWhile the basic ARIA implementation is good, consider adding more descriptive labels.
<Button variant="ghost" className="focus:ring-0 px-2 ml-1" size="sm" + aria-label={t("open_search_options")} onClick={() => setOpen(true)} >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/SearchByMultipleFields.tsx(7 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (1)
src/components/Common/SearchByMultipleFields.tsx (1)
70-70: LGTM! Improved state managementGood improvements:
- Edge case handling for initialOptionIndex
- Performance optimization with memoized unselectedOptions
Also applies to: 104-107
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Changes
OrganizationSelectorwithGovtOrganizationSelectorin various forms and components.shortcutKeyproperties from search components, impacting keyboard shortcut functionality.