Replace the magnify-and-new-tab figure with an inline lightbox - #3313
Replace the magnify-and-new-tab figure with an inline lightbox#3313enf0rc3 wants to merge 5 commits into
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 |
| // Fetching the full size on the way to the click means the zoom does not | ||
| // spend its first frames showing the column-sized version stretched | ||
| const warm = () => { | ||
| const full = largest(img); |
There was a problem hiding this comment.
This is the wrong way to go about it.
Our responsive image thing renders image tags like this:
<img src="/docs/i/x/getting-started/dashboard.png" alt="Octopus Dashboard" srcset="/docs/i/600/getting-started/dashboard.webp 600w, /docs/i/1000/getting-started/dashboard.webp 1000w, /docs/i/2000/getting-started/dashboard.webp 1500w" sizes="(min-width: 1680px) 1000px, (min-width: 940px) calc(71.81vw - 192px), calc(100vw - 32px)" class="resp-img" width="1500" height="736" style="--shown: 1;" tabindex="0">
The original/largest image is just in the src attribute. We should ignore the srcset when looking to swap in a full-sized replacement
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