Replace the magnify-and-new-tab figure with an inline lightbox - #3313
Conversation
|
Pull request environment is available at https://stoctodocspr3313.z22.web.core.windows.net. You can view the ephemeral environment status in Octopus Deploy. This environment will be automatically deprovisioned when the pull request is closed, or after 7 days of inactivity. |
|
So exciting! This looks fantastic 🤩 @enf0rc3 I've realised we probably need a border because some images are white 😅 |
5fe96fc to
4fde6b1
Compare
Clicking the magnifier called window.open, which dropped the reader on a bare image file and lost the page they were reading. The frame around each figure - 2rem of padding, a navy border and a grey panel - was duplicated in three places, because markdown figures and the Image component were styled separately. The image now carries the interaction itself: click it, and it animates from where it sits to a fit-viewport rect over an opaque backdrop, matching the design in NES-284. Click or Escape closes it. tabindex is what a wrapping button would otherwise have given for free, and the zoom would be mouse-only without it. Modelled on Linear's docs, which the designer referenced. Their whole lightbox is seven CSS rules and no library, and PhotoSwipe would have been 50KB of which most gets switched off again: the design has no close button, no counter, no arrows and no zoom controls. No markup changes, so the emitted HTML is unchanged and nothing about how images are served moves. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
4fde6b1 to
8dfe780
Compare
|
Looking good! Some UI feedback
Taking the image-swapping, close and scroll-bugs into consideration, I feel like photoswipe is probably worthwhile. |
Light screenshots have no visible edge against the page, and the lightbox makes that worse by putting them on an opaque background of the same colour. Applied to the zoomed image as well, for the same reason. Only html is border-box, so the images also need it explicitly. Without it the border sits outside the max-width: 100% they inherit from the global img rule and overflows the column by 2px. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three things reviewers hit on the first version. The page scrolled behind the lightbox. Locking overflow on the document normally reflows everything by the width of the scrollbar, so the gutter is now reserved permanently and hiding it costs nothing. There was no way out except the cursor or Escape, and both are easy to miss. A close button sits in the top right and takes focus when the lightbox opens, which also gives Tab somewhere to land. The zoom took whichever candidate the browser had picked for a column and stretched it. The clone now names the widest file in the srcset outright, and hovering an image fetches it, so by the time a click lands it is already decoded. On the badges page that is a 1432px file behind a 1340px box instead of a 1000px one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed the border, did a spike to implement the library but required way more customisation, was easier just to do the fixes recommended. |
It lived until the element was removed at the end of the animation, so it hung in the corner for the whole 400ms shrink. Reads as the lightbox ignoring the click. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@enf0rc3 I had a play with the latest version. The "X" button is cool, and the under-scrolling bug is fixed, and the subtle border looks nice The image swapping doesn't seem to be there though. I went to the main page on /docs. The responsive image for the dashboard gets loaded into the page via |
The generated variants stop at 2000px, so picking the widest one threw away most of a 3326px screenshot: /docs opened overview.png at 2000px when the original had 3326. src is the untouched original and is never smaller, which matters most on a 2x display, where a 1500 CSS-px lightbox wants 3000 real pixels. Comparing against the width attribute rather than always taking src keeps the saving where a variant does match the original's width. The dashboard image is 1500px either way, so it opens as a 67KB webp instead of a 183KB png. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
src is the original. The srcset only holds variants generated for a column, capped at 2000px, so the widest of them lost 1326px of a 3326px screenshot. Picking between them meant trusting the width attribute, and this repo has already shipped sidecars that lied about it - the environment-timeline pair recorded 1000x535 for images that are 1644x1081. A wrong width would have quietly chosen the smaller file with nothing to catch it. Reading src cannot be wrong, and it stays right if the variants change. It costs the webp saving where a variant matches the original's width, which is about 124KB across the two images that qualify, and only on click. Comments trimmed while I was in here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Hi @borland I have made the changes regarding the full src.
This one doesn't have an image larger than 1500 so you would need to start on a smaller browser window to recreate this one e.g: The image below this one on the dashboard page goes up to 3k and works. |
4 participants
Closes NES-284.
Clicking the magnifier called
window.open, dropping the reader on a bare image file. Figure images now click to zoom in place.Modelled on Linear's docs, which the designer referenced, and matching the Figma frames under Lightbox (
1848:17946,1848:18049), plus a close button.Two files
No markup changes — the emitted HTML is byte-identical to
main. Nothing about how images are served changes: no new files, no image-pipeline changes,public/untouched.No new dependency
I started on PhotoSwipe, then read Linear's shipped CSS — their whole lightbox is 7 rules and no vendor library.
I did go back and build the PhotoSwipe version properly once review raised scroll-locking and the close button — it's on
willlaugesen/nes-284-photoswipe-spikeand it works. We're not taking it yet, because this version animatesleft/top/width/heightrather thantransform: scale(), and that difference matters: a transform scales its whole subtree, so the 8px radius drew at 43px mid-zoom and the 1px border at 5px, both snapping back at the end. Avoiding that in PhotoSwipe needs counter-scaling the radius per frame. Here it's free.The spike stays available if we later want pinch-zoom on touch, which this doesn't do.
How it works
figures.jsreads the image's rect, clones it into a fixed frame at that rect, then animates to a fit-viewport rect over an opaquevar(--color-base-primary)backdrop — already theme-aware, so dark mode is free.prefers-reduced-motionskips the animation.The image carries the interaction directly. Linear uses a wrapping
<div aria-label="Zoom image">, which isn't focusable and gives keyboard users nothing;img.tabIndex = 0plus an Enter/Space handler is what a wrapper would otherwise provide, without touching the markup.main.cssloses the.imageframe, the.image__imgshadow, the legacyfigure:has(p > img) pduplicate (which carried aTODOto remove it), and all six.magnify-*rules. What replaces it is onefigure imgrule covering both the markdown and<Image>paths — the duplication thatTODOwas waiting on is gone without migrating 1,452 blocks to a component.Review feedback, all addressed
overflow: hiddenplus permanentscrollbar-gutter: stable, so hiding the scrollbar reflows nothingsrcsetfile outright, and hovering prefetches itborder: var(--borderWidth1) solid var(--colorBorderPrimary), inline and zoomedVerified
Full build (2,673 pages, clean), then driven with Playwright against the built site, including under 300KB/s with 300ms latency:
zoom-inrgb(255,255,255)light,rgb(17,26,35)darkzoom-out#DEE1E6light,#2E475Ddark, no overflow (799.97px in an 800px column):::figureand the<Image>MDX componentWorth knowing
Mobile pinch-zoom. The Figma frames are 1920×1080 only. This fits to viewport and stops there, so a dense screenshot is still hard to read on a phone. Linear has the same limitation. The PhotoSwipe spike solves it without changing this markup.
naturalWidthlies on srcset images. It returns a density-adjusted size — the badges thumbnail reports 842 where the file is genuinely 1432. It nearly read as an upscaling bug. The pages withsrcsetalso carry a realwidthattribute, which takes priority in the upscale cap, so behaviour is correct.Upscale cap. A narrow screenshot opens at its true pixel width rather than stretching, which keeps it sharp.
🤖 Generated with Claude Code