Fix FileUploadDialog overflow | Use ShadCN component #10047
Conversation
WalkthroughThe pull request modifies the Changes
Assessment against linked issues
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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Files/FilesTab.tsx (1)
605-633: Well-structured form field implementation with ShadCN components!The implementation:
- Maintains accessibility with proper labels and ARIA attributes
- Handles error states effectively
- Uses consistent styling
Consider adding data-testid attributes for better test coverage:
<Input name={`file_name_${index}`} type="text" id={`upload-file-name-${index}`} + data-testid={`upload-file-name-${index}`} required className={`block w-full rounded-md border px-3 py-2 text-sm ${ index === 0 && fileUpload.error ? "border-red-500 focus:ring-red-500" : "border-gray-300 focus:ring-primary" }`} value={fileUpload.fileNames[index] || ""}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Files/FilesTab.tsx(3 hunks)
🔇 Additional comments (2)
src/components/Files/FilesTab.tsx (2)
22-23: LGTM! Clean import of ShadCN components.The addition of Input and Label components from the UI library aligns with the PR objective to use ShadCN components.
580-580: Good fix for the dialog overflow!The
max-w-fitclass allows the dialog to adapt its width based on content, preventing overflow issues.
Co-authored-by: Rithvik Nishad <rithvikn2001@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Files/FilesTab.tsx (1)
628-632: Enhance error message accessibility.Consider adding
aria-invalidandaria-errormessageattributes to improve accessibility of the error state.<Input name={`file_name_${index}`} type="text" id={`upload-file-name-${index}`} required className={ index === 0 && fileUpload.error ? "border-red-500 focus:ring-red-500" : "border-gray-300 focus:ring-primary" } + aria-invalid={index === 0 && fileUpload.error ? "true" : undefined} + aria-errormessage={index === 0 && fileUpload.error ? `error-${index}` : undefined} value={fileUpload.fileNames[index] || ""} disabled={fileUpload.uploading} onChange={(e) => fileUpload.setFileName(e.target.value, index) } /> {index === 0 && fileUpload.error && ( - <p className="mt-1 text-sm text-red-500"> + <p id={`error-${index}`} className="mt-1 text-sm text-red-500"> {fileUpload.error} </p> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Files/FilesTab.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: cypress-run (1)
🔇 Additional comments (3)
src/components/Files/FilesTab.tsx (3)
22-23: LGTM! Clean import of ShadCN components.The addition of Input and Label components from the UI library aligns with the PR objective of using ShadCN components.
605-633: Clean implementation of ShadCN components.The replacement of TextFormField with Label and Input components is well-structured and maintains all necessary functionality while following the configured styles.
580-580: Dialog width adjustment looks good.The addition of
max-w-fitclass should resolve the overflow issues by allowing the dialog to adjust its width based on content.Let's check if there are any other dialogs in the codebase that might benefit from this fix:
✅ Verification successful
Dialog width variations are intentional and appropriate
The
max-w-fitclass in FilesTab is specific to its use case, while other dialogs in the codebase intentionally use different width constraints based on their content and purpose (e.g.,sm:max-w-mdfor notes, no constraints for commands). This is the correct approach as dialog widths should be tailored to their specific content requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other DialogContent components that might need width adjustment ast-grep --pattern 'DialogContent className="$_"'Length of output: 51
Script:
#!/bin/bash # Search for Dialog-related components and their className usage rg -i "dialog.*content.*class" -A 2 -B 2 # Also search for specific Dialog imports to understand the component usage rg -i "import.*dialog" -A 2Length of output: 6983
|
This doesn't really solve the issue if file name is long 🤔 
|
I have fixed this overfitting issue, please check again. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/components/Files/FilesTab.tsx (1)
594-594: Consider using relative width for file name truncation.Using
max-w-xsmight be too restrictive. Consider using a percentage-based width or a larger fixed width to better utilize available space.- <span className="truncate max-w-xs">{file.name}</span> + <span className="truncate max-w-[calc(100%-2rem)]">{file.name}</span>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Files/FilesTab.tsx(4 hunks)
🔇 Additional comments (2)
src/components/Files/FilesTab.tsx (2)
22-23: LGTM! Using ShadCN components.The imports align with the PR objective of using ShadCN UI components.
605-633: LGTM! Well-structured form field implementation.The replacement of TextFormField with Input and Label components is well-implemented:
- Proper error handling and display
- Consistent styling that matches configured styles
- Clear labeling and accessibility attributes
Should add tool tip (that displays the full name) like elsewhere, when we are using truncate (in most cases). @coderabbitai should make this suggestion for future. |
|
Tip For best results, initiate chat on the files or code changes. @Jacobjeevan Thank you for the feedback. I'll remember to suggest adding tooltips whenever text truncation is used in the UI. 🐰 thoughtfully taking notes ✏️ Learnings added
|
|
Tip For best results, initiate chat on the files or code changes. @Jacobjeevan Thank you for the feedback. I'll remember to suggest adding tooltips whenever text truncation is used in the UI. 🐰 thoughtfully taking notes ✏️ Learnings added
|
Jacobjeevan
removed
question
…-dev/care_fe into issue/10046/modal-overflow
made the changes as requested, |
|
Tip For best results, initiate chat on the files or code changes. @rajku-dev The tooltip implementation for long filenames looks good! The conditional rendering based on filename length and the use of ShadCN's Tooltip components is well done. The changes effectively address the original issue with file name overflow. 🔗 Analysis chain🏁 Scripts executedThe following scripts were executed for the analysis: Script: Length of output: 22653 If you found this review helpful, would you consider giving us a shout-out on X? |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/components/Files/FilesTab.tsx (1)
607-635: Consider adding aria-invalid and aria-errormessage for better accessibility.The form field implementation looks good, but could be enhanced for screen readers.
<Input name={`file_name_${index}`} type="text" id={`upload-file-name-${index}`} required + aria-invalid={index === 0 && !!fileUpload.error} + aria-errormessage={index === 0 && fileUpload.error ? `error-${index}` : undefined} className={ index === 0 && fileUpload.error ? "border-red-500 focus:ring-red-500" : "border-gray-300 focus:ring-primary" } value={fileUpload.fileNames[index] || ""} disabled={fileUpload.uploading} onChange={(e) => fileUpload.setFileName(e.target.value, index) } /> {index === 0 && fileUpload.error && ( - <p className="mt-1 text-sm text-red-500"> + <p id={`error-${index}`} className="mt-1 text-sm text-red-500"> {fileUpload.error} </p> )}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/components/Files/FilesTab.tsx(4 hunks)
🧰 Additional context used
📓 Learnings (1)
src/components/Files/FilesTab.tsx (1)
Learnt from: rajku-dev
PR: ohcnetwork/care_fe#10047
File: src/components/Files/FilesTab.tsx:580-580
Timestamp: 2025-01-20T09:46:46.025Z
Learning: In FileUploadDialog, using `max-w-fit` on the dialog content is acceptable when the internal content (like filenames) is already constrained with classes like `max-w-xs` and `truncate`.
🔇 Additional comments (3)
src/components/Files/FilesTab.tsx (3)
22-23: LGTM! Clean import additions.The new imports for ShadCN components follow the project's conventions.
580-580: LGTM! Appropriate width constraint.The
max-w-fitclass is correctly used here since the internal content (filename) is already constrained withmax-w-xsandtruncate.
594-596: LGTM! Good handling of long filenames.The implementation properly handles long filenames with:
- Width constraint using
max-w-xs- Truncation with
truncate- Accessibility support via
titleattribute
|
LGTM |
Proposed Changes
BEFORE
AFTER
@ohcnetwork/care-fe-code-reviewers
Merge Checklist
Summary by CodeRabbit
UI Improvements
Styling Updates