-
Notifications
You must be signed in to change notification settings - Fork 516
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
Feat: Update the design and shortcut key for SearchByMultipleOptions #9851
base: develop
Are you sure you want to change the base?
Feat: Update the design and shortcut key for SearchByMultipleOptions #9851
Conversation
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 reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (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. |
@AdityaJ2305 I think you can make use of |
Fixed: #9867 (comment) |
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
🧹 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
13
should 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. |
91e9334
to
3cfb24a
Compare
|
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: 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
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: 1
🧹 Nitpick comments (2)
src/components/Common/SearchByMultipleFields.tsx (2)
117-124
: Consider adding ARIA attributes for keyboard shortcutsWhile the keyboard shortcut implementation is good, consider adding ARIA attributes to improve accessibility.
+ aria-keyshortcuts="Control+K"
391-403
: Consider adding keyboard shortcut for clear searchThe clear search button is a good addition, but consider adding a keyboard shortcut (e.g., Esc when the input is focused) for better user experience.
useEffect(() => { const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === "Escape" && document.activeElement === inputRef.current) { + setSearchValue(""); + return; + } // ... existing code }; }, []);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Common/SearchByMultipleFields.tsx
(6 hunks)
🔇 Additional comments (5)
src/components/Common/SearchByMultipleFields.tsx (5)
31-31
: LGTM! Platform-specific import addedThe addition of
isAppleDevice
utility enables platform-specific keyboard shortcut display.
45-45
: Breaking Change:initialOptionIndex
is now requiredThis change was previously flagged and remains a concern.
104-107
: LGTM! Efficient memoizationThe memoization of unselectedOptions is well-implemented and will prevent unnecessary recalculations.
178-215
: LGTM! Well-implemented device-specific UIThe keyboard shortcut display is properly implemented with device-specific considerations and good visual feedback.
Also applies to: 219-250
70-70
: Consider edge case handling for initialOptionIndexThe fallback to 0 when initialOptionIndex is -1 might not be the best approach. Consider throwing an error or providing better validation.
✅ Verification successful
Fallback to 0 for initialOptionIndex is appropriate
The current implementation is correct and safe because:
- The component requires at least one search option to function
- The fallback ensures a valid initial state for the selectedOption reference
- The component already has proper error handling for invalid inputs
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of SearchByMultipleFields to verify initialOptionIndex values rg -A 2 "initialOptionIndex={" --type ts --type tsxLength of output: 157
Script:
#!/bin/bash # Find usages of SearchByMultipleFields component rg -l "SearchByMultipleFields" --type ts --type tsx # Get the full implementation of SearchByMultipleFields cat src/components/Common/SearchByMultipleFields.tsxLength of output: 14004
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
🧹 Nitpick comments (2)
src/components/Users/UserResetPassword.tsx (2)
228-231
: Consider adding loading spinner for better UX.While the button text changes to "updating", consider adding a loading spinner icon for better visual feedback.
<Button type="submit" disabled={!form.formState.isDirty || isPending} variant="primary" > + {isPending && <CareIcon icon="l-spinner" className="mr-2 h-4 w-4 animate-spin" />} {isPending ? t("updating") : t("update_password")} </Button>
Line range hint
41-137
: Consider rate limiting password attempts.The password reset functionality could benefit from rate limiting to prevent brute force attempts. Consider implementing a mechanism to track failed attempts and temporarily lock the account after a certain threshold.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Users/UserResetPassword.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Redirect rules - care-ohc
- GitHub Check: Header rules - care-ohc
- GitHub Check: Pages changed - care-ohc
- GitHub Check: cypress-run (1)
- GitHub Check: OSSAR-Scan
🔇 Additional comments (2)
src/components/Users/UserResetPassword.tsx (2)
228-228
: LGTM! Good improvement to prevent double submissions.The addition of
isPending
to the button's disabled state is a good practice that prevents duplicate form submissions while the password reset is in progress.
Line range hint
89-91
: Verify password reset mutation error handling.The mutation setup doesn't include error handling. Ensure that API errors are properly caught and displayed to the user.
|
@rithviknishad @Jacobjeevan Ready for Review |
👋 Hi, @AdityaJ2305, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
👋 Hi, @AdityaJ2305, This message is automatically generated by prince-chrismc/label-merge-conflicts-action so don't hesitate to report issues/improvements there. |
Proposed Changes
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
Release Notes
New Features
Improvements
Bug Fixes
Changes
OrganizationSelector
withGovtOrganizationSelector
in various forms and components.shortcutKey
properties from search components, impacting keyboard shortcut functionality.