Conversation
| export const PreviewGroup = ({ | ||
| currentIndex, | ||
| imageItems, | ||
| mediaItems, | ||
| previewVisible, | ||
| setCurrentIndex, | ||
| setPreviewVisible, | ||
| setSelectedMedia, | ||
| }: ImagePreviewGroupProps) => { |
There was a problem hiding this comment.
Moved from ImagesAndVideos
Strictly speaking this component doesn't get re-used so doesn't deserve to be a component, but I was trying to reduce line count and separate concerns in ImagesAndVideo it does get rendered in two places, but is the same in both.
| import { PreviewGroup } from "./ImagePreviewGroup"; | ||
| import { MediaCard } from "./MediaCard"; |
There was a problem hiding this comment.
I pulled the preview component, and MediaCard (along with renderMediaItems() into separate files to make this a little less intimidating to read.
There was a problem hiding this comment.
Pull request overview
This PR implements swipable media cards for small screens by refactoring the ImagesAndVideos component into a more modular structure. The changes introduce responsive behavior that displays media in a horizontally scrollable card layout on tablet and mobile devices, while maintaining the original layout for larger screens.
Changes:
- Refactored
ImagesAndVideoscomponent into separate, focused modules (MediaCard,ImagePreviewGroup, and main component) - Added responsive swipable card layout for tablet/mobile viewports with scroll-snap behavior
- Updated padding and styling to support the new mobile card experience
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/templates/disease-cell-line.tsx | Updated import path to reflect new component directory structure |
| src/templates/cell-line.tsx | Updated import path to reflect new component directory structure |
| src/style/layout.module.css | Changed content padding from 0 to 8px on small screens |
| src/style/index.sass | Added global overscroll prevention for vertical scrolling |
| src/style/images-and-videos.module.css | Added extensive mobile/tablet styling for swipable cards and updated media query breakpoints |
| src/components/ImagesAndVideos.tsx | Removed entire file as part of refactoring into modular components |
| src/components/ImagesAndVideo/MediaCard.tsx | New component handling individual media card rendering |
| src/components/ImagesAndVideo/ImagesAndVideos.tsx | New main component with responsive layout logic |
| src/components/ImagesAndVideo/ImagePreviewGroup.tsx | New component handling image preview functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gap: 20px; | ||
| overflow-x: auto; | ||
| scroll-snap-type: x mandatory; | ||
| -webkit-overflow-scrolling: touch; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| videoIframe, | ||
| } = require("../../style/images-and-videos.module.css"); | ||
|
|
||
| interface MobileImageCardProps { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| {mediaItems.map((_item, index) => ( | ||
| <MediaCard | ||
| className={mobileMediaCard} | ||
| key={index} | ||
| index={index} | ||
| title={title} | ||
| isMobile={isMobile} | ||
| item={mediaItems[index]} |
There was a problem hiding this comment.
The _item parameter is unused (indicated by the underscore prefix) but then mediaItems[index] is accessed on line 170. Consider removing the underscore and using the item parameter directly instead of re-accessing the array by index.
| {mediaItems.map((_item, index) => ( | |
| <MediaCard | |
| className={mobileMediaCard} | |
| key={index} | |
| index={index} | |
| title={title} | |
| isMobile={isMobile} | |
| item={mediaItems[index]} | |
| {mediaItems.map((item, index) => ( | |
| <MediaCard | |
| className={mobileMediaCard} | |
| key={index} | |
| index={index} | |
| title={title} | |
| isMobile={isMobile} | |
| item={item} |
There was a problem hiding this comment.
I read this and took a stab at it and found it buggy, but I'll try again.
There was a problem hiding this comment.
what was buggy about it? it should be the exact some item right?
There was a problem hiding this comment.
Idk what I did last time but it worked this time! Pushed this a couple days agio:
ef492c5
TravisKroeker
left a comment
There was a problem hiding this comment.
approving in anticipation of merging the changes we discussed
✅ Deploy Preview for cell-catalog ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
@interim17: checking on this: the description still says "DRAFT" and Travis seems to have asked for changes. Are those things both addressed? |
|
@meganrm yes and those changes were merged |
There was a problem hiding this comment.
this is a nice encapsulation
| {parentalGeneSymbol | ||
| ? `${geneSymbol} in WTC-${fluorescentTag}-${parentalGeneSymbol} (${alleleTag}-allelic tag)` | ||
| : `${geneSymbol} in WTC-${fluorescentTag} (${alleleTag}-allelic tag)`} |
There was a problem hiding this comment.
I'm guessing this is how we had this before, but it looks like something that could be a formatting function instead of inline like this
There was a problem hiding this comment.
Easy to make one, I think it feels a bit odd to pass that many props to a util so I'm going to define the util above the render in this file.
meganrm
left a comment
There was a problem hiding this comment.
a few small issues otherwise, looks good
| className={classNames( | ||
| mobileMediaCard, | ||
| mediaItems.length === 1 && singleImage, | ||
| )} |
There was a problem hiding this comment.
last commit added a couples rules to center and fill the space if there is only one image, it looked funny offset to the left
Problem
Solution
Screenshots (optional):