-
Notifications
You must be signed in to change notification settings - Fork 524
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: PatientNoteCard Text Overflowing and Current Address #9593
Fix: PatientNoteCard Text Overflowing and Current Address #9593
Conversation
WalkthroughThe pull request encompasses modifications across multiple components, focusing on data handling and UI improvements. Changes include refactoring the 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. |
@AdityaJ2305 the shift+enter shortcut to send message in doctor notes are not working |
@AdityaJ2305 allow with the issue with your PR bug, fix another one more UI issue in the patient details page, i have updated the issue, look into it |
onKeyDown={(e) => { | ||
if (e.key === "Enter" && !e.shiftKey && !e.metaKey) { | ||
e.stopPropagation(); | ||
} | ||
}} |
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.
although it may solve the issue, why do we need these hacks? can't we stick with html standards? solve the root cause :) this is just hiding the root cause
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.
The condition is added to handle different behaviors for components like DiscussNote (where Shift + Enter should send a message) and forms like Add Patient (where just Enter should submit the form). By stopping the propagation only when Enter is pressed, it ensures both use cases work as intended without interference.
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.
are you sure that textarea element triggers a "submit" event when pressing enter in the first place? i don't think the issue is with TextAreaFormField itself in the first place.
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.
The Add Patient
submit action gets trigger on pressing enter
key
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.
care_fe/src/components/Form/Form.tsx
Line 102 in 37dce64
if (e.key === "Enter") { |
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 is textarea form field responsible for it?
do try replicating this with simple html textarea and form component. pressing enter on textarea does not trigger form submit by itself.
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/components/Patient/PatientHome.tsx (1)
39-39
: Use of multiple button components.You're importing both
ButtonV2
here andButton
from"@/components/ui/button"
. If there is no specific need for both, consider consolidating to a single button component to reduce confusion and ensure consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Patient/PatientHome.tsx
(4 hunks)
🔇 Additional comments (2)
src/components/Patient/PatientHome.tsx (2)
Line range hint 544-567
: Confirm consistent authorization handling.
Double-check that switching from AuthorizedButton
to ButtonV2
still restricts transfer actions to authorized users. If ButtonV2
depends on separate logic (e.g., a context or a custom hook) to handle role checks, verify that it’s configured.
Line range hint 526-539
: Successful replacement with ButtonV2
.
Replacing the volunteer assignment button with ButtonV2
looks good. Ensure you have tested any role or permission differences if ButtonV2
behaves differently from the old button component (e.g., if authorizeFor
logic differs).
✅ Verification successful
Let me analyze the usage of ButtonV2
and its authorizeFor
prop in the codebase.
Let me try a different approach to understand the ButtonV2 component and its authorization handling.
Let me check the ButtonV2 component implementation directly.
Based on the ButtonV2 implementation, I can now provide a final response about the authorization behavior:
The authorizeFor
prop is properly handled in ButtonV2
The ButtonV2 component correctly handles the authorizeFor
prop by:
- Using the
AuthorizedChild
component to wrap the button whenauthorizeFor
is provided - Automatically disabling the button when the user is not authorized
- Preserving the same behavior as the original button component
The implementation shows that the authorization logic is consistent and properly integrated, making the replacement of the volunteer assignment button with ButtonV2 safe from an authorization perspective.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Confirms usage of `authorizeFor` with ButtonV2.
# We expect references in code to confirm consistent usage.
rg 'ButtonV2' -A 5 -B 5
Length of output: 83264
Script:
#!/bin/bash
# Search for the ButtonV2 component definition to understand how authorizeFor is handled
ast-grep --pattern 'interface ButtonV2Props {
$$$
authorizeFor?: $_
$$$
}'
# Search for authorizeFor implementation
ast-grep --pattern 'function $_($_: ButtonV2Props) {
$$$
authorizeFor
$$$
}'
Length of output: 167
Script:
#!/bin/bash
# Search for ButtonV2 component file
fd ButtonV2.tsx
# Look for authorizeFor usage in ButtonV2 implementation
rg -A 10 -B 10 'authorizeFor.*=.*props\.authorizeFor'
Length of output: 108
Script:
#!/bin/bash
# Get the ButtonV2 component implementation
cat src/components/Common/ButtonV2.tsx
Length of output: 7095
@AdityaJ2305 what is the status on this PR |
In progress, I am discussing about it with rithvik on slack |
@nihal467, ready for review and testing |
Closing as solved in #9607 |
Proposed Changes
Previous:-
Fix:
@ohcnetwork/care-fe-code-reviewers
@nihal467
Merge Checklist
Summary by CodeRabbit
Release Notes
UI Improvements
Behavior Changes
These updates focus on improving user interface readability and interaction consistency across various components.