feat: "Feature Request: Bulk Image Upload Capability in Media Popup" #398
Conversation
|
Someone is attempting to deploy a commit to the Listinai Team on Vercel. A member of the Team first needs to authorize it. |
| @@ -0,0 +1,3 @@ | |||
| { | |||
| "WillLuke.nextjs.hasPrompted": true | |||
There was a problem hiding this comment.
Please remove this file from the PR.
WalkthroughThis pull request introduces a new VS Code setting in Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant PC as PostCreator
participant MP as MediaPopup
U->>PC: Click "Add Images" button
PC->>MP: Open MediaPopup with image gallery
U->>MP: Click to select/deselect images
MP->>U: Update selection feedback
U->>MP: Confirm selection with "Add Images" button
MP->>PC: Return selected images
PC->>U: Display updated image list in the post
Poem
Tip 🌐 Web search-backed reviews and chat
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| }`} | ||
| onClick={() => toggleImageSelection(image)} | ||
| > | ||
| <img src={image} alt="media" /> |
Check warning
Code scanning / ESLint
Prevent usage of `<img>` element due to slower LCP and higher bandwidth. Warning
| <img | ||
| key={image} | ||
| src={image} | ||
| alt="Selected" | ||
| className="selected-image" | ||
| /> |
Check warning
Code scanning / ESLint
Prevent usage of `<img>` element due to slower LCP and higher bandwidth. Warning
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
apps/frontend/src/components/PostCreator.tsx (2)
8-14: Consider loading image gallery data dynamically.The
imageGalleryarray is currently hardcoded with static image paths. Consider loading this data dynamically from an API or configuration file for better maintainability.- const imageGallery = [ - 'image1.jpg', - 'image2.jpg', - 'image3.jpg', - 'image4.jpg', - // Add more image URLs or file paths here - ]; + const [imageGallery, setImageGallery] = useState<string[]>([]); + + useEffect(() => { + // Load images from API or configuration + const loadImages = async () => { + try { + const response = await fetch('/api/media/images'); + const data = await response.json(); + setImageGallery(data); + } catch (error) { + console.error('Failed to load images:', error); + } + }; + loadImages(); + }, []);
16-20: Consider adding error handling for image selection.The
handleAddImagescallback should include error handling for cases where image selection fails or invalid images are selected.const handleAddImages = (images: string[]) => { + try { + // Validate images (e.g., check file types, sizes) + const validImages = images.filter(image => { + const extension = image.split('.').pop()?.toLowerCase(); + return ['jpg', 'jpeg', 'png', 'gif'].includes(extension || ''); + }); + + if (validImages.length !== images.length) { + console.warn('Some images were skipped due to invalid file types'); + } + - setSelectedImages((prevImages) => [...prevImages, ...images]); + setSelectedImages((prevImages) => [...prevImages, ...validImages]); setShowMediaPopup(false); + } catch (error) { + console.error('Failed to add images:', error); + // Handle error (e.g., show error message to user) + } };apps/frontend/src/components/MediaPopup.tsx (2)
3-6: Consider adding additional props for customization.The component could benefit from additional props for customization, such as:
- Maximum number of selectable images
- Custom styling options
- Accessibility labels
interface MediaPopupProps { images: string[]; onAddImages: (selectedImages: string[]) => void; + maxSelectable?: number; + className?: string; + labels?: { + title?: string; + addButton?: string; + }; }
11-24: Add maximum selection limit and selection order.The image selection logic should include:
- Maximum selection limit
- Selection order tracking for better UX
- const toggleImageSelection = (image: string) => { + const toggleImageSelection = (image: string) => { + const maxSelectable = props.maxSelectable || Infinity; + if (selectedImages.includes(image)) { setSelectedImages(selectedImages.filter((img) => img !== image)); } else { + if (selectedImages.length >= maxSelectable) { + // Show error message or handle max limit + return; + } setSelectedImages([...selectedImages, image]); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.vscode/settings.json(1 hunks)apps/frontend/src/components/MediaPopup.tsx(1 hunks)apps/frontend/src/components/PostCreator.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .vscode/settings.json
🧰 Additional context used
🪛 GitHub Check: ESLint
apps/frontend/src/components/PostCreator.tsx
[warning] 30-35: Prevent usage of <img> element due to slower LCP and higher bandwidth.
Using <img> could result in slower LCP and higher bandwidth. Consider using <Image /> from next/image to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element
apps/frontend/src/components/MediaPopup.tsx
[warning] 38-38: Prevent usage of <img> element due to slower LCP and higher bandwidth.
Using <img> could result in slower LCP and higher bandwidth. Consider using <Image /> from next/image to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build (20.17.0)
| {selectedImages.map((image) => ( | ||
| <img | ||
| key={image} | ||
| src={image} | ||
| alt="Selected" | ||
| className="selected-image" | ||
| /> | ||
| ))} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace <img> with Next.js Image component for better performance.
Using the <img> element can result in slower LCP (Largest Contentful Paint) and higher bandwidth usage. Consider using Next.js's Image component for automatic image optimization.
+import Image from 'next/image';
- {selectedImages.map((image) => (
- <img
- key={image}
- src={image}
- alt="Selected"
- className="selected-image"
- />
- ))}
+ {selectedImages.map((image) => (
+ <Image
+ key={image}
+ src={image}
+ alt="Selected"
+ width={100}
+ height={100}
+ className="selected-image"
+ objectFit="cover"
+ />
+ ))}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {selectedImages.map((image) => ( | |
| <img | |
| key={image} | |
| src={image} | |
| alt="Selected" | |
| className="selected-image" | |
| /> | |
| ))} | |
| import Image from 'next/image'; | |
| // ... other imports and code above remain unchanged | |
| {selectedImages.map((image) => ( | |
| <Image | |
| key={image} | |
| src={image} | |
| alt="Selected" | |
| width={100} | |
| height={100} | |
| className="selected-image" | |
| objectFit="cover" | |
| /> | |
| ))} |
🧰 Tools
🪛 GitHub Check: ESLint
[warning] 30-35: Prevent usage of <img> element due to slower LCP and higher bandwidth.
Using <img> could result in slower LCP and higher bandwidth. Consider using <Image /> from next/image to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element
| }`} | ||
| onClick={() => toggleImageSelection(image)} | ||
| > | ||
| <img src={image} alt="media" /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace <img> with Next.js Image component.
Similar to the previous file, replace the <img> element with Next.js's Image component for better performance.
+import Image from 'next/image';
- <img src={image} alt="media" />
+ <Image
+ src={image}
+ alt="media"
+ width={100}
+ height={100}
+ objectFit="cover"
+ />📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <img src={image} alt="media" /> | |
| import Image from 'next/image'; | |
| ... | |
| <Image | |
| src={image} | |
| alt="media" | |
| width={100} | |
| height={100} | |
| objectFit="cover" | |
| /> | |
| ... |
🧰 Tools
🪛 GitHub Check: ESLint
[warning] 38-38: Prevent usage of <img> element due to slower LCP and higher bandwidth.
Using <img> could result in slower LCP and higher bandwidth. Consider using <Image /> from next/image to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element
| return ( | ||
| <div className="media-popup"> | ||
| <h3>Select Images</h3> | ||
| <div className="image-grid"> | ||
| {images.map((image) => ( | ||
| <div | ||
| key={image} | ||
| className={`image-item ${ | ||
| selectedImages.includes(image) ? 'selected' : '' | ||
| }`} | ||
| onClick={() => toggleImageSelection(image)} | ||
| > | ||
| <img src={image} alt="media" /> | ||
| </div> | ||
| ))} | ||
| </div> | ||
| <button onClick={handleAddClick} disabled={selectedImages.length === 0}> | ||
| Add {selectedImages.length > 0 ? `(${selectedImages.length})` : ''}{' '} | ||
| Images | ||
| </button> | ||
|
|
||
| <style jsx>{` | ||
| .media-popup { | ||
| padding: 20px; | ||
| } | ||
| .image-grid { | ||
| display: grid; | ||
| grid-template-columns: repeat(auto-fill, minmax(100px, 1fr)); | ||
| gap: 10px; | ||
| } | ||
| .image-item { | ||
| cursor: pointer; | ||
| border: 2px solid transparent; | ||
| transition: border-color 0.3s ease; | ||
| } | ||
| .image-item img { | ||
| max-width: 100px; | ||
| height: auto; | ||
| } | ||
| .image-item.selected { | ||
| border-color: blue; /* Border for selected images */ | ||
| } | ||
| button { | ||
| margin-top: 20px; | ||
| padding: 10px; | ||
| background-color: #007bff; | ||
| color: white; | ||
| border: none; | ||
| cursor: pointer; | ||
| border-radius: 5px; | ||
| } | ||
| button:disabled { | ||
| background-color: grey; | ||
| cursor: not-allowed; | ||
| } | ||
| `}</style> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add keyboard navigation and accessibility features.
The component should support keyboard navigation and include proper ARIA attributes for better accessibility.
return (
- <div className="media-popup">
+ <div
+ className="media-popup"
+ role="dialog"
+ aria-labelledby="media-popup-title"
+ >
- <h3>Select Images</h3>
+ <h3 id="media-popup-title">Select Images</h3>
<div className="image-grid">
{images.map((image) => (
<div
key={image}
className={`image-item ${
selectedImages.includes(image) ? 'selected' : ''
}`}
onClick={() => toggleImageSelection(image)}
+ onKeyDown={(e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ toggleImageSelection(image);
+ }
+ }}
+ role="button"
+ tabIndex={0}
+ aria-pressed={selectedImages.includes(image)}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return ( | |
| <div className="media-popup"> | |
| <h3>Select Images</h3> | |
| <div className="image-grid"> | |
| {images.map((image) => ( | |
| <div | |
| key={image} | |
| className={`image-item ${ | |
| selectedImages.includes(image) ? 'selected' : '' | |
| }`} | |
| onClick={() => toggleImageSelection(image)} | |
| > | |
| <img src={image} alt="media" /> | |
| </div> | |
| ))} | |
| </div> | |
| <button onClick={handleAddClick} disabled={selectedImages.length === 0}> | |
| Add {selectedImages.length > 0 ? `(${selectedImages.length})` : ''}{' '} | |
| Images | |
| </button> | |
| <style jsx>{` | |
| .media-popup { | |
| padding: 20px; | |
| } | |
| .image-grid { | |
| display: grid; | |
| grid-template-columns: repeat(auto-fill, minmax(100px, 1fr)); | |
| gap: 10px; | |
| } | |
| .image-item { | |
| cursor: pointer; | |
| border: 2px solid transparent; | |
| transition: border-color 0.3s ease; | |
| } | |
| .image-item img { | |
| max-width: 100px; | |
| height: auto; | |
| } | |
| .image-item.selected { | |
| border-color: blue; /* Border for selected images */ | |
| } | |
| button { | |
| margin-top: 20px; | |
| padding: 10px; | |
| background-color: #007bff; | |
| color: white; | |
| border: none; | |
| cursor: pointer; | |
| border-radius: 5px; | |
| } | |
| button:disabled { | |
| background-color: grey; | |
| cursor: not-allowed; | |
| } | |
| `}</style> | |
| </div> | |
| return ( | |
| <div | |
| className="media-popup" | |
| role="dialog" | |
| aria-labelledby="media-popup-title" | |
| > | |
| <h3 id="media-popup-title">Select Images</h3> | |
| <div className="image-grid"> | |
| {images.map((image) => ( | |
| <div | |
| key={image} | |
| className={`image-item ${selectedImages.includes(image) ? 'selected' : ''}`} | |
| onClick={() => toggleImageSelection(image)} | |
| onKeyDown={(e) => { | |
| if (e.key === 'Enter' || e.key === ' ') { | |
| toggleImageSelection(image); | |
| } | |
| }} | |
| role="button" | |
| tabIndex={0} | |
| aria-pressed={selectedImages.includes(image)} | |
| > | |
| <img src={image} alt="media" /> | |
| </div> | |
| ))} | |
| </div> | |
| <button onClick={handleAddClick} disabled={selectedImages.length === 0}> | |
| Add {selectedImages.length > 0 ? `(${selectedImages.length})` : ''} Images | |
| </button> | |
| <style jsx>{` | |
| .media-popup { | |
| padding: 20px; | |
| } | |
| .image-grid { | |
| display: grid; | |
| grid-template-columns: repeat(auto-fill, minmax(100px, 1fr)); | |
| gap: 10px; | |
| } | |
| .image-item { | |
| cursor: pointer; | |
| border: 2px solid transparent; | |
| transition: border-color 0.3s ease; | |
| } | |
| .image-item img { | |
| max-width: 100px; | |
| height: auto; | |
| } | |
| .image-item.selected { | |
| border-color: blue; /* Border for selected images */ | |
| } | |
| button { | |
| margin-top: 20px; | |
| padding: 10px; | |
| background-color: #007bff; | |
| color: white; | |
| border: none; | |
| cursor: pointer; | |
| border-radius: 5px; | |
| } | |
| button:disabled { | |
| background-color: grey; | |
| cursor: not-allowed; | |
| } | |
| `}</style> | |
| </div> | |
| ); |
🧰 Tools
🪛 GitHub Check: ESLint
[warning] 38-38: Prevent usage of <img> element due to slower LCP and higher bandwidth.
Using <img> could result in slower LCP and higher bandwidth. Consider using <Image /> from next/image to automatically optimize images. This may incur additional usage or cost from your provider. See: https://nextjs.org/docs/messages/no-img-element
What kind of change does this PR introduce?
Why was this change needed?
Other information:
Summary by CodeRabbit