Fix responsive layout: desktop scroll bug, mobile flow, touch targets - #11
Fix responsive layout: desktop scroll bug, mobile flow, touch targets#11rowkav09 wants to merge 6 commits into
Conversation
dev_server.log was a stray output file committed by accident; it has no purpose in version control.
The three-column grid used the default align-items: stretch, so the preview panel was forced to match the tallest sidebar's natural height (~2100px), leaving roughly 800px of empty black space around a much smaller rendered image and requiring the whole page to scroll well past the viewport on ordinary desktop sizes. Give the workspace a fixed viewport height on lg+ screens and let each panel (settings, preview, export) scroll independently instead. This also replaces the xl-only (1280px) column breakpoint with a lg one (1024px) using clamp()-based sidebar widths, so common laptop sizes like 1024x768 get a real multi-column layout instead of one full-width stacked column.
On phones the page previously stacked every desktop panel in document order with the preview shown before the upload control, and buried Export beneath ~2150px of renderer/image/background/colour controls (12+ range sliders with no grouping). That made Export - the action used every single time - the hardest thing to reach. - Drop the CSS order hack that showed the preview before the upload dropzone; content now follows the natural Upload -> Adjust -> Inspect -> Export order at every breakpoint. - Group "Image controls" and "Background" behind a collapsible <details> section (closed by default), since they are the least frequently touched controls and by far the tallest. This cuts total mobile page height by roughly a quarter and puts Export right after the preview instead of at the very bottom. - Fix a latent type hole this surfaced: updateAdjustment took `value: number` for every ImageAdjustments key, so the one non-numeric key (fitMode) was silently passed through an `as any` cast. Made it generic over the key so the value type is checked per-field.
Every button measured 38px tall and range sliders had only a 6px hit box - both below the ~44px minimum recommended for touch. Applied min-h-11 across buttons and selects, widened range slider tracks (h-1.5/h-2 to h-6), and enlarged colour swatches (32px to 40px tall). Toggle switches (Invert, Foreground/background separation) and the range slider track height are pragmatic exceptions: sized up from their originals but kept below 44px, since forcing a switch or a fine-adjustment slider to a full 44px square would look oversized next to their labels without meaningfully improving usability - the switch and slider thumb, not the track, is what gets touched.
The only prior way to clear a loaded image was to reload the page - "Reset image settings" only reset sliders, not the image itself. Add a Remove button next to Browse (shown once an image is loaded) that revokes the object URL and returns the preview to its empty state.
- text-slate-500 on the app's black background measures ~4.4:1, just under the 4.5:1 WCAG AA threshold for the small (10-12px) label and status text it was used for everywhere. Move to text-slate-400 (~8:1) throughout. - The dither-compare modal took focus nowhere on open and released it nowhere on close, and Tab could escape onto controls hidden behind the overlay. Focus the Close button on open, cycle Tab within the dialog while it's open, and return focus to the triggering button on close.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe editor now provides responsive desktop layouts, larger touch targets, collapsible image settings, image removal cleanup, stronger adjustment typing, and keyboard focus management for the comparison dialog. Development server logs are excluded from version control. ChangesImage editor UI
Development log exclusion
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant ComparisonTrigger
participant ComparisonDialog
participant CloseButton
ComparisonTrigger->>ComparisonDialog: Open comparison dialog
ComparisonDialog->>CloseButton: Focus close button
ComparisonDialog->>ComparisonDialog: Trap Tab focus
CloseButton->>ComparisonDialog: Close dialog
ComparisonDialog->>ComparisonTrigger: Restore focus
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/page.tsx`:
- Around line 306-317: Update the page-level keyboard shortcut handler near the
showDitherCompare dialog logic to return immediately whenever showDitherCompare
is true, before processing the R, C, or U shortcuts. Gate that handler directly
rather than relying on propagation control from the dialog’s later window
listener, while preserving shortcut behavior when the dialog is closed.
- Line 754: Reorder the grid children around the main layout container so the
mobile stacking order is Upload, all adjustment controls (Renderer and Image),
preview/Inspect, then Export. Update the aside section identified around the
affected grid items, preserving the existing desktop column placement and
responsive behavior.
- Line 308: Update the selector string in the dialog focusable-elements query to
use double quotes, while retaining single quotes around the nested tabindex
attribute value; leave the selector contents and querySelectorAll call
unchanged.
- Around line 439-450: Update removeImage to also increment renderRequestRef and
clear isRendering during image cleanup, invalidating any pending renderArt
request so it cannot restore removed art or state.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e97b0ab5-795e-4ab4-bd03-dde01002211a
⛔ Files ignored due to path filters (1)
dev_server.logis excluded by!**/*.log
📒 Files selected for processing (2)
.gitignoresrc/app/page.tsx
| if (event.key === "Escape") { onClose(); return; } | ||
| if (event.key !== "Tab") return; | ||
| const focusable = dialogRef.current?.querySelectorAll<HTMLElement>('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'); | ||
| if (!focusable?.length) return; | ||
| const first = focusable[0]; | ||
| const last = focusable[focusable.length - 1]; | ||
| if (event.shiftKey && document.activeElement === first) { event.preventDefault(); last?.focus(); } | ||
| else if (!event.shiftKey && document.activeElement === last) { event.preventDefault(); first?.focus(); } | ||
| }; | ||
| window.addEventListener("keydown", onKeyDown); | ||
| return () => window.removeEventListener("keydown", onKeyDown); | ||
| }, [onClose]); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent page shortcuts while the dialog is open.
The page listener that starts at Line 707 registers R, C, and U shortcuts before this modal listener. Those shortcuts still run when focus is in the modal. R can reset settings and U can open the upload picker behind an aria-modal dialog.
Gate the page shortcut handler on showDitherCompare. Do not rely on propagation control in this later window listener.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/page.tsx` around lines 306 - 317, Update the page-level keyboard
shortcut handler near the showDitherCompare dialog logic to return immediately
whenever showDitherCompare is true, before processing the R, C, or U shortcuts.
Gate that handler directly rather than relying on propagation control from the
dialog’s later window listener, while preserving shortcut behavior when the
dialog is closed.
| if (event.key === "Escape") onClose(); | ||
| if (event.key === "Escape") { onClose(); return; } | ||
| if (event.key !== "Tab") return; | ||
| const focusable = dialogRef.current?.querySelectorAll<HTMLElement>('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a double-quoted selector string.
Line 308 uses a single-quoted string. Use a double-quoted string and single quotes for the nested attribute value.
Proposed fix
- const focusable = dialogRef.current?.querySelectorAll<HTMLElement>('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])');
+ const focusable = dialogRef.current?.querySelectorAll<HTMLElement>("button, [href], input, select, textarea, [tabindex]:not([tabindex='-1'])");As per coding guidelines: Use two-space indentation, semicolons, double-quoted strings, and Tailwind utility classes in TSX.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const focusable = dialogRef.current?.querySelectorAll<HTMLElement>('button, [href], input, select, textarea, [tabindex]:not([tabindex="-1"])'); | |
| const focusable = dialogRef.current?.querySelectorAll<HTMLElement>("button, [href], input, select, textarea, [tabindex]:not([tabindex='-1'])"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/page.tsx` at line 308, Update the selector string in the dialog
focusable-elements query to use double quotes, while retaining single quotes
around the nested tabindex attribute value; leave the selector contents and
querySelectorAll call unchanged.
Source: Coding guidelines
| const removeImage = useCallback(() => { | ||
| loadRequestRef.current += 1; | ||
| if (fileUrlRef.current) URL.revokeObjectURL(fileUrlRef.current); | ||
| fileUrlRef.current = null; | ||
| imageRef.current = null; | ||
| setLoadedImage(null); | ||
| setImageUrl(null); | ||
| setImageReady(false); | ||
| setGeneratedArt(null); | ||
| setShowComparison(false); | ||
| setStatus("Image removed. Choose another image or switch to text."); | ||
| }, []); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Invalidate pending renders when removing an image.
removeImage invalidates only loadRequestRef. If renderArt is awaiting generateArtFromImage, its request remains current and it can restore removed art, status, and rendering state after this cleanup.
Increment renderRequestRef and clear isRendering when removing the image.
Proposed fix
const removeImage = useCallback(() => {
loadRequestRef.current += 1;
+ renderRequestRef.current += 1;
if (fileUrlRef.current) URL.revokeObjectURL(fileUrlRef.current);
fileUrlRef.current = null;
imageRef.current = null;
setLoadedImage(null);
setImageUrl(null);
setImageReady(false);
+ setIsRendering(false);
setGeneratedArt(null);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const removeImage = useCallback(() => { | |
| loadRequestRef.current += 1; | |
| if (fileUrlRef.current) URL.revokeObjectURL(fileUrlRef.current); | |
| fileUrlRef.current = null; | |
| imageRef.current = null; | |
| setLoadedImage(null); | |
| setImageUrl(null); | |
| setImageReady(false); | |
| setGeneratedArt(null); | |
| setShowComparison(false); | |
| setStatus("Image removed. Choose another image or switch to text."); | |
| }, []); | |
| const removeImage = useCallback(() => { | |
| loadRequestRef.current += 1; | |
| renderRequestRef.current += 1; | |
| if (fileUrlRef.current) URL.revokeObjectURL(fileUrlRef.current); | |
| fileUrlRef.current = null; | |
| imageRef.current = null; | |
| setLoadedImage(null); | |
| setImageUrl(null); | |
| setImageReady(false); | |
| setIsRendering(false); | |
| setGeneratedArt(null); | |
| setShowComparison(false); | |
| setStatus("Image removed. Choose another image or switch to text."); | |
| }, []); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/page.tsx` around lines 439 - 450, Update removeImage to also
increment renderRequestRef and clear isRendering during image cleanup,
invalidating any pending renderArt request so it cannot restore removed art or
state.
|
|
||
| <div className="grid flex-1 gap-4 xl:grid-cols-[292px_minmax(0,1fr)_292px]"> | ||
| <section className="order-2 space-y-3 rounded-md border border-white/10 bg-white/[0.03] p-3 backdrop-blur-sm xl:order-1"> | ||
| <div className="grid flex-1 gap-4 lg:min-h-0 lg:grid-cols-[clamp(240px,22vw,300px)_minmax(0,1fr)_clamp(240px,22vw,300px)]"> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Restore the required mobile workflow order.
Below the lg breakpoint, the grid stacks the left section, preview, and then this aside. This aside renders Export before Renderer and Image controls. Mobile users therefore reach Inspect and Export before all adjustments.
Reorganize the mobile layout so all adjustment controls precede the preview and Export follows it. This conflicts with the stated Upload → Adjust → Inspect → Export workflow.
Also applies to: 783-794
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/app/page.tsx` at line 754, Reorder the grid children around the main
layout container so the mobile stacking order is Upload, all adjustment controls
(Renderer and Image), preview/Inspect, then Export. Update the aside section
identified around the affected grid items, preserving the existing desktop
column placement and responsive behavior.
1 participant
What was broken
align-items: stretch, so the preview panel was forced to match the tallest sidebar's height (~2100px). At 1440x900 the preview panel was 2178px tall while the actual rendered content was 483px, centered in ~800px of empty black space, and the whole page required scrolling to ~2350px on a 900px viewport.xl(1280px), so a common size like 1024x768 got one full-width column with a two-button toggle stretched to ~970px.text-slate-500labels measured ~4.4:1 contrast against the black background, just under WCAG AA's 4.5:1 for small text.No horizontal page overflow was found anywhere in the original code, at any viewport - that part already worked and wasn't touched beyond preserving it.
What changed
lg:and up, 1024px+) is now a fixed-height workspace: header + a100dvh-bounded row where settings, preview, and export each scroll independently instead of the whole document stretching. Sidebar width isclamp(240px, 22vw, 300px)instead of a fixed 292px only above 1280px.orderput the preview before the upload control). The bulkiest control clusters ("Image controls", "Background") are grouped into closed-by-default<details>sections, cutting total mobile page height by roughly a quarter and moving Export right after the preview instead of the very bottom.min-h-11(44px); slider tracks widened from 6-8px to 24px. Toggle switches and slider track thickness are a deliberate exception (see commit message) rather than forced to exactly 44px.text-slate-500→text-slate-400(~8:1 contrast) for all labels/status text.updateAdjustmentwas typed fornumberonly, so the one string-valued setting relied on anas anycast) - made it generic per-key.dev_server.log.Testing
npm run lint,npm run typecheck,npm test(22 tests),npm run buildall pass.503from/api/uses(no local Upstash Redis configured, which is documented existing behavior).Known limitations
CONFLICTINGwithmainand a prior session'sdocs/branch-audit.mdalready recorded that its useful ideas were folded into merged PRs feat: improve dithering visual quality and add gradient backgrounds #9/Persist background settings in links #10. Left untouched since closing PRs isn't something I do without being asked.Summary by CodeRabbit