🎨 Palette: Improve file upload UX with dynamic icons and size formatting#604
🎨 Palette: Improve file upload UX with dynamic icons and size formatting#604
Conversation
- Added `frontend/src/utils/formatters.ts` for file size formatting (bytes to KB/MB/GB). - Updated `ConversionUploadEnhanced.tsx` to use the new formatter. - Added dynamic file icons (`.jar` -> ☕, `.zip` -> 🗜️). - Enhanced drag-and-drop feedback text. - Verified changes with Playwright and unit tests. Co-authored-by: anchapin <6326294+anchapin@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
There was a problem hiding this comment.
Pull request overview
This PR enhances the file upload user experience in the ConversionUploadEnhanced component by adding dynamic file type icons, human-readable file size formatting, and improved drag-and-drop feedback.
Changes:
- Added a new
formatFileSizeutility function to format file sizes in human-readable units (Bytes, KB, MB, GB, TB) - Implemented dynamic file icons that display different emojis based on file type (☕ for .jar, 🗜️ for .zip, 📦 for others)
- Enhanced drag-and-drop UX with contextual text that changes to "Drop file to upload 📂" when actively dragging
- Updated tests to properly wrap components in
ProgressProviderand improved WebSocket mock return values
Reviewed changes
Copilot reviewed 3 out of 6 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pnpm-lock.yaml | Minor storybook version update from 10.2.9 to 10.2.10 |
| frontend/src/utils/formatters.ts | New utility file with formatFileSize function for human-readable file size display |
| frontend/src/components/ConversionUpload/ConversionUploadEnhanced.tsx | Added getFileIcon function, integrated formatFileSize, added dynamic drag feedback text, improved accessibility with aria-hidden attributes |
| frontend/src/components/ConversionUpload/ConversionUploadEnhanced.test.tsx | Wrapped test components in ProgressProvider, updated WebSocket mocks to return cleanup functions |
Files not reviewed (1)
- pnpm-lock.yaml: Language not supported
| export const formatFileSize = (bytes: number): string => { | ||
| if (bytes === 0) return '0 Bytes'; | ||
| const k = 1024; | ||
| const sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB']; | ||
| const i = Math.floor(Math.log(bytes) / Math.log(k)); | ||
| return parseFloat((bytes / Math.pow(k, i)).toFixed(2)) + ' ' + sizes[i]; | ||
| }; |
There was a problem hiding this comment.
The formatFileSize function is duplicated across the codebase in at least 5 different locations (ConversionAssetDetails.tsx, ConversionAssetsUpload.tsx, ConversionHistory/utils.ts, urlParser.ts, and now formatters.ts). This creates a maintainability issue where bug fixes or improvements need to be applied in multiple places. Consider consolidating all instances to use the new utility in formatters.ts and removing the duplicated implementations. This would be especially beneficial since the implementations are slightly different - some use 'Bytes' while others use 'B', and the ConversionHistory version always shows MB regardless of size.
| export const formatFileSize = (bytes: number): string => { | ||
| if (bytes === 0) return '0 Bytes'; | ||
| const k = 1024; | ||
| const sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB']; | ||
| const i = Math.floor(Math.log(bytes) / Math.log(k)); | ||
| return parseFloat((bytes / Math.pow(k, i)).toFixed(2)) + ' ' + sizes[i]; |
There was a problem hiding this comment.
The formatFileSize function doesn't handle negative byte values. While file sizes should never be negative in practice, defensive programming suggests either validating the input (throwing an error or returning an error message like "Invalid size") or documenting that the function expects non-negative values. Currently, a negative value would produce an incorrect result like "-1.00 KB" due to the Math.log calculation with negative numbers.
| const k = 1024; | ||
| const sizes = ['Bytes', 'KB', 'MB', 'GB', 'TB']; | ||
| const i = Math.floor(Math.log(bytes) / Math.log(k)); | ||
| return parseFloat((bytes / Math.pow(k, i)).toFixed(2)) + ' ' + sizes[i]; |
There was a problem hiding this comment.
The new formatFileSize utility function lacks test coverage. Consider adding unit tests to verify edge cases such as: zero bytes, very small files (< 1KB), boundary values between units (e.g., 1023 bytes vs 1024 bytes), large files (GB and TB ranges), and the formatting precision (2 decimal places). This is especially important since the function will likely be reused across the codebase to replace duplicated implementations.
| return parseFloat((bytes / Math.pow(k, i)).toFixed(2)) + ' ' + sizes[i]; | |
| const value = (bytes / Math.pow(k, i)).toFixed(2); | |
| return `${value} ${sizes[i]}`; |
| @@ -442,10 +449,10 @@ export const ConversionUploadEnhanced: React.FC<ConversionUploadProps> = ({ | |||
| </div> | |||
| ) : selectedFile ? ( | |||
| <div className="file-preview"> | |||
| <div className="file-icon">📦</div> | |||
| <div className="file-icon" aria-hidden="true">{getFileIcon(selectedFile.name)}</div> | |||
| <div className="file-info"> | |||
| <div className="file-name">{selectedFile.name}</div> | |||
| <div className="file-size">{(selectedFile.size / 1024 / 1024).toFixed(2)} MB</div> | |||
| <div className="file-size">{formatFileSize(selectedFile.size)}</div> | |||
| <div className="status">{getStatusMessage()}</div> | |||
| </div> | |||
| <button | |||
| @@ -463,8 +470,8 @@ export const ConversionUploadEnhanced: React.FC<ConversionUploadProps> = ({ | |||
| </div> | |||
| ) : ( | |||
| <div className="upload-prompt initial-prompt"> | |||
| <div className="upload-icon-large">☁️</div> | |||
| <h3>Drag & drop your modpack here</h3> | |||
| <div className="upload-icon-large" aria-hidden="true">☁️</div> | |||
| <h3>{isDragActive ? "Drop file to upload 📂" : "Drag & drop your modpack here"}</h3> | |||
There was a problem hiding this comment.
The new dynamic file icon feature (getFileIcon function) and drag feedback text changes lack test coverage. The existing tests have been updated to wrap components in ProgressProvider, but no tests verify that: 1) the coffee emoji (☕) appears for .jar files, 2) the clamp emoji (🗜️) appears for .zip files, 3) the default package emoji (📦) appears for other files, or 4) the drag-active text "Drop file to upload 📂" appears when dragging files. Consider adding tests to verify these new UX improvements work correctly.
Improved the file upload experience in
ConversionUploadEnhanced.tsxby adding:.jarfiles and a Clamp icon for.zipfiles, giving immediate visual feedback.formatFileSizeutility to display file sizes in KB, MB, or GB instead of raw bytes or fixed MB.Verified with Playwright (screenshots generated) and existing unit tests (updated to wrap in
ProgressProvider).PR created automatically by Jules for task 8726980627345380001 started by @anchapin