feat(core): add XDSLightbox component - #2298
Conversation
|
@czarandy must be a member of the Meta Open Source team on Vercel to deploy. Learn more about collaboration on Vercel and other options here. |
b289152 to
f417df3
Compare
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR Analysis Report📚 Storybook PreviewView Storybook for this PR 🧪 Sandbox PreviewView Sandbox for this PR No new or modified components detected. Bundle Size Summary
Accessibility AuditStatus: No accessibility violations detected. Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
cixzhang
left a comment
There was a problem hiding this comment.
The API is clean and the implementation is solid, but this needs to go through the full specification protocol before merging. Issue #1198 says "Specification protocol in progress" but it was never completed.
Specifically:
-
External research + use case analysis — need a proper survey of Fancybox, PhotoSwipe, Radix Dialog patterns, etc. and a decision matrix. The current API is reasonable but decisions need to be grounded.
-
Vibe tests — especially around the
mediasingle-vs-array overload. Does an AI naturally reach formedia={{src, alt}}vsmedia={[{src, alt}]}? Does it correctly wireindex/onIndexChangefor galleries? -
Missing slots — real-world lightboxes need toolbar actions (download, share, open in tab). The spec should surface this and decide on a slot/render prop pattern. Also
captionshould beReactNodenot just string. -
Trigger pattern — XDSDialog uses
useXDSDialoghook with a trigger ref. Lightbox should match:useXDSLightboxreturning trigger props + state, consistent with the dialog family. -
Video autoPlay — currently hardcoded to
true. Should behasAutoPlaydefaulting tofalseper boolean conventions.
The code quality and conventions compliance are good — this is purely about running the spec protocol to validate the API surface before shipping.
f417df3 to
249f0eb
Compare
249f0eb to
495bc47
Compare
|
I did look at Fancybox, PhotoSwipe and Lightbox2 to base this version on. I'm not going to do the "decision matrix" or whatever.
|
|
⏺ ## Lightbox Vibe Test Results All 3 Lightbox vibe tests completed.
Key findings
|
495bc47 to
9a6fa09
Compare
This is probably fine, we don't really care that much which version gets used, it's just in case. |
|
Imperative hook form is pretty typical for our layers. It's not about trigger positioning but rather so we have a way to open layers from things like menus where the items might unmount because the menu closes after showing the layer |
| } | ||
|
|
||
| export const Default: Story = { | ||
| render: () => <SingleImageDemo />, |
There was a problem hiding this comment.
For these storybook examples it would be ideal to render them directly in the render methods instead of wrapping in a separate component because this obfuscates the code examples
Fullscreen overlay for viewing images and videos at full resolution. - Unified media prop: single object or array for gallery mode - Video support: type='video' renders <video> with native controls - Gallery mode: prev/next buttons and arrow key navigation - Zoom: opt-in via hasZoom (images only), double-click toggles 1x/2x, drag to pan - Built on native <dialog> with showModal() for focus trapping - Controls use XDSIconButton with XDSIcon (close, chevronLeft/Right) - Dark backdrop, white controls via --color-on-dark token - Caption hugs media, gallery counter at top-left - Accessible: aria-label, keyboard nav, focus restore on close - Exports: XDSLightbox, XDSLightboxProps, XDSLightboxMedia, XDSLightboxMediaType
9a6fa09 to
f934473
Compare
|
Added useXDSLightbox hook + fix the other comments |
🚀 Vercel Preview Deployment
|
Fullscreen overlay for viewing images and videos at full resolution. - Unified media prop: single object or array for gallery mode - Video support: type='video' renders <video> with native controls - Gallery mode: prev/next buttons and arrow key navigation - Zoom: opt-in via hasZoom (images only), double-click toggles 1x/2x, drag to pan - Built on native <dialog> with showModal() for focus trapping - Controls use XDSIconButton with XDSIcon (close, chevronLeft/Right) - Dark backdrop, white controls via --color-on-dark token - Caption hugs media, gallery counter at top-left - Accessible: aria-label, keyboard nav, focus restore on close - Exports: XDSLightbox, XDSLightboxProps, XDSLightboxMedia, XDSLightboxMediaType
Fullscreen overlay for viewing images and videos at full resolution. - Unified media prop: single object or array for gallery mode - Video support: type='video' renders <video> with native controls - Gallery mode: prev/next buttons and arrow key navigation - Zoom: opt-in via hasZoom (images only), double-click toggles 1x/2x, drag to pan - Built on native <dialog> with showModal() for focus trapping - Controls use XDSIconButton with XDSIcon (close, chevronLeft/Right) - Dark backdrop, white controls via --color-on-dark token - Caption hugs media, gallery counter at top-left - Accessible: aria-label, keyboard nav, focus restore on close - Exports: XDSLightbox, XDSLightboxProps, XDSLightboxMedia, XDSLightboxMediaType
Summary
New
XDSLightboxcomponent — a fullscreen overlay for viewing images and videos at full resolution.src,alt,caption,typepropsimages[],index,onIndexChangewith prev/next buttons and arrow key navigationtype='video'renders<video>with native controls; zoom/pan disabled for videohasZoom(images only) — double-click toggles 1x/2x, drag to pan when zoomed<dialog>withshowModal()for focus trapping and top-layerXDSIconButtonwith<XDSIcon>, white via--color-on-darktokenaria-label, keyboard navigation, focus restore on closeXDSLightbox,XDSLightboxProps,XDSLightboxImage,XDSLightboxMediaTypeTest plan
tsc --noEmit