feat: add variants to Photo Gallery #993
Conversation
commit: |
WalkthroughThis PR adds gallery and carousel display variants to PhotoGallerySection: new locale keys (carousel, gallery, carouselImageCount) across platform locales; a migration to set a default "carousel" variant when absent; a styles.variant property (default "gallery") with resolveData synchronization; a refactor of PhotoGalleryWrapper to render either a responsive carousel or a gallery grid (including carouselImageCount and a ResizeObserver); updated tests and documentation; and an ImageList default placeholder image addition. Sequence Diagram(s)sequenceDiagram
participant Editor
participant PhotoGallerySection
participant MigrationRegistry
participant PhotoGalleryWrapper
participant CarouselProvider
participant ResizeObserver
Editor->>MigrationRegistry: load/migrate page data
MigrationRegistry->>PhotoGallerySection: ensure default variant (carousel) if missing
Editor->>PhotoGallerySection: render (styles.variant)
PhotoGallerySection->>PhotoGallerySection: resolveData() sync wrapper parentData.variant
PhotoGallerySection->>PhotoGalleryWrapper: render with parentData.variant & styles.carouselImageCount
alt variant == "carousel"
PhotoGalleryWrapper->>CarouselProvider: initialize carousel with visibleSlides
PhotoGalleryWrapper->>ResizeObserver: observe container width
ResizeObserver-->>PhotoGalleryWrapper: update visibleSlides (1 on narrow, carouselImageCount on wide)
CarouselProvider-->>PhotoGalleryWrapper: render visible slides
else variant == "gallery"
PhotoGalleryWrapper-->>PhotoGalleryWrapper: render GalleryGrid (all images)
end
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/visual-editor/locales/platform/hu/visual-editor.json`:
- Line 203: Replace the English value for the key "carouselImageCount" with the
Hungarian translation; locate the "carouselImageCount" entry in
visual-editor.json and change its value to a Hungarian string (e.g., "Karusszel
képek száma") maintaining the JSON string syntax and trailing comma formatting
to match surrounding entries.
🧹 Nitpick comments (1)
packages/visual-editor/src/components/pageSections/PhotoGallerySection/PhotoGalleryWrapper.tsx (1)
157-368: Consider extracting the shared "Add Image" button logic.Both
DesktopImageItemandMobileImageItemhave nearly identical empty state rendering with the same event handler logic to prevent Puck drag interference. Consider extracting a sharedEmptyImagePlaceholdercomponent to reduce duplication.♻️ Example extraction
interface EmptyImagePlaceholderProps { width?: number; aspectRatio?: number; originalIndex: number; onEmptyImageClick: (e: React.MouseEvent | undefined, index: number) => void; className?: string; style?: React.CSSProperties; } const EmptyImagePlaceholder = ({ width, aspectRatio, originalIndex, onEmptyImageClick, className, style, }: EmptyImagePlaceholderProps) => { const handleButtonClick = (e: React.MouseEvent) => { e.stopPropagation(); e.preventDefault(); onEmptyImageClick(e, originalIndex); }; // ... shared event handlers and button rendering };
There was a problem hiding this comment.
Is it by design to have the arrows now be inside the section instead of having the imagine fill up the whole space (aligned with header) like previously?
There was a problem hiding this comment.
Hmmm it looks like yes based on mocks but I'll double check as well
| isEditing: boolean; | ||
| onEmptyImageClick: (e: React.MouseEvent | undefined, index: number) => void; | ||
| }) => { | ||
| if (imageData.isEmpty && isEditing) { |
There was a problem hiding this comment.
Can this be shared between mobile & desktop?
There was a problem hiding this comment.
should all the arrows/dots be grey here?
There was a problem hiding this comment.
Yup! Only 3 pics so since all 3 are in this "slide" the dots are all grayed out (dots correspond to each image) and there's no more pics to use the arrows to view.
| image={imageData.image} | ||
| aspectRatio={imageData.aspectRatio} | ||
| className="w-full h-auto object-contain" | ||
| sizes={`${Math.min(imageData.width || 1000, 250)}px`} |
There was a problem hiding this comment.
I think this can just be 100vw... I guess the carousel images on mobile are less than 100vw but it's probably close enough?
| "rounded-image-borderRadius", | ||
| constrainToParent && "w-full h-auto object-contain max-w-full" | ||
| )} | ||
| sizes={`min(${imageWidth}px, calc(100vw - 6rem))`} |
There was a problem hiding this comment.
This should probably depend on carousel/gallery
(this is a hint to the browser of how wide the image is so it can download the minimally necessary resolution)
Default screenshots changed as the new default is Gallery variant.
Existing versions are migrated to Carousel variant with 1 carousel image per slide.
Dot size has changed per SUMO-8096 and we now handle carousels with lots of images:
if too many for the dots with min-width 24px, hide the dots.
