Repository navigation
[companion] fix(image-viewer): open fitted to the screen, with Fit and 1:1 zoom - #85
andrebrait wants to merge 5 commits into
Conversation
The lightbox opened at 100% on every device and zoomed with a transform that did not grow the scroll area. Open fitted to the viewport (never upscaling small images), show zoom relative to real pixels, step by x1.25 down to min(10%, fit), size the image by layout so every edge scrolls into view, and replace Reset zoom with Fit to screen and 1:1.
Keep the centre of the view across zoom changes, show an image that fails to load (or has no intrinsic size) instead of leaving it hidden, size only once both the image and the viewport are measured, and give the viewer buttons 44px targets under the Touch Targets setting.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (7)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe image lightbox now opens images fitted to the available area and supports stepped zoom, fit-to-screen, and actual-size controls. It sizes images for scrolling, preserves the viewport center during zoom, and updates control styling, touch-target sizing, and translations. ChangesImage lightbox zoom
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
actor Viewer
participant ImageLightbox
participant Image
participant ResizeObserver
participant Viewport
Image->>ImageLightbox: report natural dimensions when loading settles
ResizeObserver->>ImageLightbox: report viewport content size
ImageLightbox->>Viewport: set image dimensions from fit scale
Viewer->>ImageLightbox: choose fit, actual size, or stepped zoom
ImageLightbox->>Viewport: update image dimensions and preserve fractional center
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The image viewer now opens images fitted to the viewport and adds stepped zoom, Fit to screen and 1:1 controls. No concrete merge-blocking risk was identified. Quickly check zoom and scroll behavior in a browser before release. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 66.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (5 skipped: 5 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
|
@codex review |
|
@coderabbitai review |
✅ Action performedReview finished.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 467d96da5a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const observer = new ResizeObserver(([entry]) => { | ||
| setBox({ width: entry.contentRect.width, height: entry.contentRect.height }); |
There was a problem hiding this comment.
When Interface Scale is Comfortable or Large, the root uses zoom: 1.1 or 1.2 (app/globals.css:622-628), while this observer measures a dialog whose unchanged dimensions are 100vw × 100dvh. Viewport units are zoom-invariant in this app and therefore paint beyond the window unless divided by --ui-scale (app/globals.css:599-607), so fitZoom fits the image into the off-screen portion of the dialog: the bottom/right can be clipped and the initial image is not actually fitted to the visible screen. Compact scale similarly leaves the viewer and image undersized. Inversely scale the lightbox's viewport-unit dimensions, as the other dialogs do, before using this measurement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 93d2d96: the viewer's 100vw/100dvh are now divided by --ui-scale, like the other dialogs. Checked at 1400×900: Compact and Large both cover exactly 1400×900, and the fitted image ends inside the window (23% / 17%).
Viewport units ignore the root zoom, so the viewer was sized past the window (Comfortable/Large) or short of it (Compact) and fit to that.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93d2d96a33
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Real layout size, not transform: scale, so a zoomed-in image | ||
| // scrolls to every edge. Hidden until it can be sized to fit. | ||
| style={ready | ||
| ? { width: natural.width * scale, height: natural.height * scale } |
There was a problem hiding this comment.
When Interface Scale is Compact, Comfortable, or Large, the root zoom still scales the dimensions assigned here, so clicking 1:1 paints a 1000-pixel-wide image at 900, 1100, or 1200 CSS pixels while the toolbar reports 100%; every other displayed percentage is similarly offset. Counteract --ui-scale when converting the natural dimensions into layout dimensions so the new actual-size control and percentages remain relative to the image's real pixels.
AGENTS.md reference: AGENTS.md:L599-L607
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63bb4de: the fit is computed in screen pixels (contentRect × --ui-scale) and the image is laid out at natural × zoom / --ui-scale. At Compact and at Large, 1:1 measures 6000 screen px for a 6000px image, and fit is about 20% at both scales on a 1400×900 window.
| html[data-touch-targets="accessible"] .image-lightbox-stepper, | ||
| html[data-touch-targets="accessible"] :is(.image-lightbox-stepper button, .image-lightbox-icon-button, .image-lightbox-zoom-value) { | ||
| min-width: 44px; | ||
| height: 44px; | ||
| line-height: 44px; |
There was a problem hiding this comment.
On a 360px-wide phone with Touch Targets set to Accessible and Interface Scale set to Large, the dialog has only about 300 unzoomed CSS pixels and the toolbar content area about 280, but these rules require roughly 288 pixels for the stepper, Fit, 1:1, Close, and their gaps even before the title. Since the stepper also has overflow: hidden and is shrinkable, its zoom controls are clipped; add a narrow responsive layout such as wrapping or a compact toolbar so all controls remain reachable.
AGENTS.md reference: AGENTS.md:L599-L607
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 63bb4de: the toolbar and its action group now wrap, and the stepper no longer shrinks. On a 360×760 phone at Large scale with Accessible targets, all five buttons are fully inside the dialog at 44px.
…lbar Measure fit and size the image in screen pixels so 1:1 and every percentage ignore the root zoom, and let the toolbar wrap so large touch targets stay reachable on narrow screens.
|
@codex review |
|
Codex Review: Didn't find any major issues. What shall we delve into next? Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Review-only companion to kahme247#226. Identical head and matching upstream base.
Codex and CodeRabbit have current availability evidence on this repository (#83). Upstream is read-only for this account, so review requests go here after upstream CI is green.
Summary by CodeRabbit