-
Notifications
You must be signed in to change notification settings - Fork 522
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
"disable "Save Button" if no file is selected." #9110 #9111
Conversation
WalkthroughThe changes in Changes
Assessment against linked issues
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
src/components/Common/AvatarEditModal.tsxOops! Something went wrong! :( ESLint: 8.57.1 Error: Failed to load parser '@typescript-eslint/parser' declared in '.eslintrc.json': Cannot find module '@typescript-eslint/parser'
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. |
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
🧹 Outside diff range and nitpick comments (2)
src/components/Common/AvatarEditModal.tsx (2)
295-295
: LGTM! Consider enhancing UX with a tooltipThe disabled condition correctly prevents form submission without file selection, addressing the issue described in #9110.
Consider adding a tooltip to provide feedback when the button is disabled:
<ButtonV2 id="save-cover-image" onClick={uploadAvatar} disabled={isProcessing || !selectedFile} + title={!selectedFile ? t("please_select_an_image_first") : ""} >
295-295
: Align disabled state handling between upload and camera modesFor consistency, consider applying the same disabled logic to the Submit button in camera capture mode.
Update the Submit button in camera capture mode:
- <Submit disabled={isProcessing} onClick={uploadAvatar}> + <Submit + disabled={isProcessing || !selectedFile} + title={!selectedFile ? t("please_capture_an_image_first") : ""} + onClick={uploadAvatar} + >Also, consider internationalizing the remaining hardcoded strings in the component for complete i18n coverage.
Also applies to: 386-398
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/components/Common/AvatarEditModal.tsx
(1 hunks)
🔇 Additional comments (1)
src/components/Common/AvatarEditModal.tsx (1)
295-295
: Verify selectedFile state management across all input methods
Let's verify that the selectedFile state is properly managed across all input methods to ensure consistent button behavior.
✅ Verification successful
Based on the results, I can now generate the final response as I have sufficient information about the selectedFile state management across all input methods.
selectedFile state is properly managed across all input paths
The verification confirms proper state management:
- File selection: Handled via input (
e.target.files[0]
) and drop events (droppedFile
) - Camera capture: Managed through
myFile
from camera component - Proper cleanup:
setSelectedFile(undefined)
called during close, invalid files, and reset scenarios - State dependencies:
selectedFile
correctly used for preview generation and upload button state
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for proper selectedFile state management
# Check file selection handling
echo "Checking file selection handling..."
rg -A 2 "setSelectedFile" src/components/Common/AvatarEditModal.tsx
# Check file clearing
echo "Checking file clearing..."
rg -A 2 "setSelectedFile\(undefined\)" src/components/Common/AvatarEditModal.tsx
# Check related state management
echo "Checking related state management..."
rg "selectedFile|setSelectedFile" src/components/Common/AvatarEditModal.tsx
Length of output: 1699
LGTM |
@Srayash Your efforts have helped advance digital healthcare and TeleICU systems. 🚀 Thank you for taking the time out to make CARE better. We hope you continue to innovate and contribute; your impact is immense! 🙌 |
Proposed Changes
Save
button if the user selects no file to update/set as facility cover.@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
New Features
Bug Fixes