-
Notifications
You must be signed in to change notification settings - Fork 667
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
Fix : Patient search bar auto focus on visiting #10736
base: develop
Are you sure you want to change the base?
Fix : Patient search bar auto focus on visiting #10736
Conversation
WalkthroughThe changes introduce an Changes
Sequence Diagram(s)sequenceDiagram
participant PI as PatientIndex
participant SMB as SearchByMultipleFields
participant PIComp as PhoneInput
PI->>SMB: Render component (autoFocus=true)
SMB->>PIComp: Pass autoFocus prop
PIComp-->>SMB: Auto-focus on render
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🪧 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. |
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 (1)
src/pages/PublicAppointments/PatientRegistration.tsx (1)
185-185
: Consider adding error handling for localStorage operations.While storing the selected patient follows the existing pattern, it's good practice to wrap localStorage operations in try-catch blocks to handle potential errors (e.g., when storage is full or disabled).
- localStorage.setItem("selectedPatient", JSON.stringify(data)); + try { + localStorage.setItem("selectedPatient", JSON.stringify(data)); + } catch (error) { + console.error("Failed to store selected patient:", error); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/components/Common/SearchByMultipleFields.tsx
(1 hunks)src/pages/PublicAppointments/PatientRegistration.tsx
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Test
- GitHub Check: cypress-run (1)
- GitHub Check: CodeQL-Build
🔇 Additional comments (4)
src/components/Common/SearchByMultipleFields.tsx (3)
39-51
: LGTM! Interface update is well-defined.The
autoFocus
prop is correctly added as an optional boolean to theSearchByMultipleFieldsProps
interface.
191-199
: LGTM! PhoneInput enhancement is properly implemented.The
autoFocus
prop is correctly passed to thePhoneInput
component.
224-228
: LGTM! Focus management is properly handled.The
useEffect
hook correctly manages focus based on theautoFocus
prop changes.src/pages/PublicAppointments/PatientRegistration.tsx (1)
181-183
: LGTM! Cache invalidation is properly implemented.The cache invalidation ensures that the patients list is refreshed after creating a new patient.
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.
pass inputRef to the component instead. autoFocus behaviour is manually controlled based on other states and side effects.
src/components/ui/phone-input.tsx
Outdated
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.
@Mahendar0701 In the deploy preview I cannot enter any phone number in entire care
@Mahendar0701 @rithviknishad the entire patient creation is broken since patient number aren't allow to enter in your PR |
src/components/ui/phone-input.tsx
Outdated
inputComponent={(inputProps) => ( | ||
<InputComponent {...inputProps} ref={inputRef} /> | ||
)} |
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.
i don't think this is a good idea. Have you checked if inputComponent itself is passing the ref? if so, we can't override it.
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.
And if i pass inputRef as prop in phone-input then a type mismatch error is coming
I think the below approach works fine, what you think? should i do this?
cc : @Jacobjeevan
<PhoneInput
id={id}
name={selectedOption.key}
placeholder={selectedOption.placeholder}
value={searchValue}
onChange={(value) => setSearchValue(value)}
className={inputClassName}
autoFocus={autoFocus}
/>
…hub.com/Mahendar0701/care_fe into patient-name-update-in-patient-switcher
@Mahendar0701 fix the cypress failure |
@Mahendar0701 Phone number is still broken. |
merge checklist not followed; |
@rithviknishad @Jacobjeevan can you reopen this PR i followed merge checklist |
@Mahendar0701, the cypress is still not fixed. can you share the EOD for this PR? |
…tRef from PhoneInput component
…hub.com/Mahendar0701/care_fe into patient-name-update-in-patient-switcher
@nihal467 all tests passing Self review: End date : today itself if everything goes well |
@nihal467 SHIFT + P works when search bar gets unfocused by clicking outside. If it is a requirement to get SHIFT+P work even on focus , then i will proceed to update this PR with that Can you plz confirm it ?? |
Since we are auto-focusing on the input, shortcut should work regardless, yes. |
@rithviknishad @Jacobjeevan i have updated the PR can you once take a look. Now keyboard shortcut works even if search bar is focused |
Proposed Changes
search.mp4
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit