74 more controls for media in lightbox#132
Conversation
[Walkthrough] Issue #121: Lightbox settings for new controlsSub-issue 1 of parent issue #74 — More controls for media in lightbox. Summary of ChangesWe implemented configuration settings for all subsequent lightbox features in issue #74 under a new "Lightbox Settings" section on the App Settings tab in 1. Data Model & Defaults (
|
|
The issue with the validation message ordering has been resolved and verified: Root Cause: The validation in page-client.tsx was checking lightboxZoomMin > 100 before checking lightboxZoomMin > lightboxZoomMax. For a test input of zoomMin = 800, the range check was triggered first, returning "Lightbox minimum zoom must be between 10% and 100%." instead of "Lightbox minimum zoom cannot be greater than maximum zoom.". Fix Applied: Reordered handleSubmit validation checks in Verification: Re-built the test container and ran npx playwright test tests/settings.spec.ts --project=chromium-serial --no-deps. All 10/10 tests passed in 5.2s. |
[Walkthrough] Issue #122: Full-height navigation zones in lightboxSub-issue 2 of parent issue #74 — More controls for media in lightbox. Summary of ChangesReplaced the circular 50x50px navigation buttons inside the lightbox media area with full-height click zones ( 1. Lightbox UI Component (
|
…t toggle and auto-hide
…uch, and item navigation events
[Walkthrough] Issue #123: Show/hide lightbox controls with auto-hideSub-issue 3 of parent issue #74 — More controls for media in lightbox. Summary of ChangesImplemented a unified show/hide controls framework ( 1. Page Data Flow & Props (
|
[Walkthrough] Issue #124: Lightbox image fit mode & zoom controlsSub-issue 4 of parent issue #74 — More controls for media in lightbox. Summary of ChangesImplemented image fit modes, interactive zoom controls, mouse wheel zooming, and drag panning inside the Lightbox. 1. Page Data Flow & Settings Props
2. Fit Modes & Zoom Controls (
|
[Walkthrough] Issue #125 (Mobile pinch-to-zoom)Summary of Accomplishments1. Image Fit Modes & Zoom Reset (
|
Code Review: PR #132PR: OverviewThis PR delivers a substantial feature set to the Lightbox media viewer: configurable fit modes, zoom controls, full-height navigation zones, auto-hide controls, mouse/touch drag panning with directional boundaries, and multi-touch pinch-to-zoom with browser zoom prevention. The commit history is clean, incremental, and follows Conventional Commits throughout. ✅ StrengthsCommit Discipline
Feature Completeness
Test Coverage
|
| Area | E2E | Unit | Notes |
|---|---|---|---|
| Navigation zones | ✅ | — | Height verification, click navigation |
| Controls toggle + H key | ✅ | — | Two-step Escape tested |
| Fit mode cycling | ✅ | — | Title attribute assertions |
| Zoom in/out/reset | ✅ | — | Percentage text assertions |
| Pinch-to-zoom | ✅ | — | Multi-touch dispatch with identifiers |
| Settings defaults | — | ✅ | 50/200/25 values validated |
| Settings UI validation | ✅ | — | Min/max boundary checks |
| Pan boundaries | — | — | Not directly tested |
| Browser zoom prevention | — | — | Hard to test in E2E without real device |
Note
Pan boundary clamping logic (clampPanOffset) has no direct test coverage. This is another argument for extracting it into a useZoomPan hook — the pure math functions could then have lightweight unit tests for each boundary mode.
Verdict
Approve with documentation follow-up. The implementation is solid, well-tested, and the commit history is exemplary. The only actionable item before merge is updating ARCHITECTURE.md and CONTRIBUTING.md per the project's own documentation synchronization rules.
Recommended Actions
| Priority | Action | Blocking? |
|---|---|---|
| High | Update ARCHITECTURE.md Lightbox entry + add Key Design Decisions |
Yes — per §2.9 rule |
| High | Update CONTRIBUTING.md settings type documentation |
Yes — per §2.9 rule |
| Low | Track useZoomPan hook extraction as a future issue |
No |
| Low | Track pan boundary unit tests as a future issue | No |
…x subsystem and design decisions
This PR will contain changes for all sub issues of #74. Walkthroughs of each implemented sub issue will be commented after committing. Sub issues will be closed as related commits are verified