[PE-102] fix: zooming for fullscreen images#6266
Conversation
WalkthroughThis pull request modifies the image handling components in the editor. It reorganizes the import statements in Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (7)
17-20: Use constants as configuration for flexible updates later
Declaring the zoom-related constants in one place is valuable for maintainability and clarity. If these values might change in the future or need to be exposed for theme/config overrides, consider centralizing them in a separate config or constants file shared across the codebase.
26-28: Consider storing magnification or position in a ref
Using a state variable for magnification forces React re-renders on every zoom adjustment. If performance becomes a concern, switching magnification handling to a ref may reduce unnecessary renders.
61-64: Allow closing even while dragging
Currently, if the user is dragging (isDragging == true), handleClose returns early. This might frustrate users who want to quickly close fullscreen mode while an accidental drag is active. Consider confirming whether preventing closure under drag is intentional.
66-85: Ensure consistent zoom experience
Using discrete zoom steps (ZOOM_STEPS) is intuitive for keyboard-based zoom adjustments. If you foresee a scenario where a user wants finer granularity—especially if the image is high resolution— you might consider a fallback continuous zoom approach or more steps.
101-120: Smooth transitions when dragging
While using "position: relative" and direct style changes is straightforward, it might produce abrupt movements if the user drags quickly. If a smooth user experience is desired, consider adding a slight transition or an easing function—though that must be weighed against making the drag feel “laggy.”
209-220: Animate left and top for a smoother reset
The CSS transition is only applied to width and transform. If you want a smooth return-to-center effect for the image, consider adding transitions for left/top as well.- transform: `scale(${magnification})`, - transition: "width 0.2s ease, transform 0.2s ease", + transform: `scale(${magnification})`, + transition: "width 0.2s ease, transform 0.2s ease, left 0.2s ease, top 0.2s ease",
224-243: Keyboard accessibility for zoom controls
The minus/plus buttons are only clickable with mouse or pointer. Consider adding keyboard navigation shortcuts or ARIA attributes to improve accessibility for these zoom controls.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/editor/src/core/extensions/custom-image/components/image-node.tsx(1 hunks)packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/editor/src/core/extensions/custom-image/components/image-node.tsx
🔇 Additional comments (4)
packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (4)
143-164: Pinch-to-zoom logic is well-structured
The approach is clean and straightforward. The clamp logic prevents overshooting the zoom limits. Keep an eye out for future requests to handle multi-finger gestures on touch devices.
190-200: Ensure background clicks are intended
You’re closing the fullscreen when the user clicks on the modal background. This is a common pattern, but be mindful if users might expect a different interaction (e.g., possibly re-centering their image). So far, it looks appropriate for a typical fullscreen experience.
✅ Verification successful
Let me verify the interaction pattern by checking if there are similar patterns in the codebase or any specific requirements defined.
This will help us understand:
- If this background click-to-close pattern is consistently used across the codebase
- If there are any documented requirements or guidelines about modal/fullscreen interactions
Let me search for more modal-related patterns to understand if this interaction is consistent with other modal implementations in the codebase.
This will help us understand:
- How other modal components handle background interactions
- If there are other fullscreen implementations with similar patterns
Let me try one more search with corrected file type.
This will help us understand:
- The modal interaction patterns across the codebase
- Any documented requirements for image interactions
Since we've been unable to find definitive patterns in the codebase about modal interactions or specific documentation about fullscreen image behavior, I'll generate the final response based on the code review context and common UX patterns.
Background click behavior follows standard modal pattern
The current implementation of closing the fullscreen view on background click follows a widely accepted modal interaction pattern. Since this is a fullscreen image viewer, clicking the background (away from the image) is an intuitive way to exit the view, and it's unlikely to conflict with other expected interactions like image re-centering, which should be handled by clicking and dragging the image itself.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for similar modal/fullscreen background click handlers
rg -U "onMouseDown.*target.*current.*(?:close|handle[A-Z])" -A 2
# Search for any documentation or comments about modal/fullscreen interaction
rg -i "fullscreen.*interaction|modal.*click|background.*click" --type md
Length of output: 562
Script:
#!/bin/bash
# Search for modal-related components and their click handlers
rg -U "Modal.*|modal.*" --type tsx -A 5
# Search for other fullscreen implementations
ast-grep --pattern 'className={$_("fixed inset-0"$$$)}'
Length of output: 127
Script:
#!/bin/bash
# Search for modal-related components and their click handlers
rg -U "Modal.*|modal.*" --type ts --type jsx --type tsx -A 5
# Look for any image-related documentation
fd -e md -x grep -l -i "image.*interaction|fullscreen.*image" {} \;
Length of output: 163
246-249: Security check for opening in new tab
While generally safe, opening external links can pose potential security or user experience concerns. If the image source can come from user input, confirm that the domain is trusted or sanitized.
✅ Verification successful
Let me investigate the source of the image to determine if it can come from user input and if there are any security measures in place.
Let me check the implementation of the custom image component to understand how the image source is handled.
Let me check one more aspect to understand how the image source is processed before being set.
Based on the code analysis, I can now generate the final response:
Image source is properly handled through controlled upload process
The image source (src) used in the full-screen view comes from a controlled upload process. The code shows that images are processed through CustomImageUploader, validated during upload, and resolved through a getImageSource function that processes the path. The source cannot be directly manipulated by users as it's handled through the editor's controlled workflow.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# First, let's find where the 'src' prop is defined and how it's handled
ast-grep --pattern 'const src = $_'
# Let's also look for any image upload or URL handling related to this component
rg -l "src.*=" packages/editor/src/core/extensions/custom-image/
# Check for any sanitization or validation of image URLs
rg -l "sanitize|validate.*url|validateUrl" packages/editor/src/core/extensions/custom-image/
Length of output: 972
Script:
#!/bin/bash
# Let's examine the custom image implementation and how src is handled
rg -A 5 "src.*=" packages/editor/src/core/extensions/custom-image/custom-image.ts
# Check the image node component implementation
cat packages/editor/src/core/extensions/custom-image/components/image-node.tsx
# Look for image upload handling
rg -A 5 "upload" packages/editor/src/core/extensions/custom-image/components/image-uploader.tsx
Length of output: 6997
Script:
#!/bin/bash
# Check the image block component to see how src is handled before being displayed
cat packages/editor/src/core/extensions/custom-image/components/image-block.tsx
# Look for any URL processing or source handling in the custom image implementation
rg -A 5 "getImageSource" packages/editor/src/core/extensions/custom-image/custom-image.ts
Length of output: 12343
36-58: Consider verifying image load completion before calculations
Here, the image dimensions are derived from raw numbers and used to compute magnification and set positions. If the image hasn’t fully loaded, these calculations might be premature for certain browsers or slow networks. Consider verifying the onLoad event before setting initial magnification/positions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (3)
17-20: Consider adjusting MIN_ZOOM for better support of large imagesThe current MIN_ZOOM value of 0.5 might be too restrictive when viewing very large images. Consider lowering it to 0.1 to allow users to zoom out further when needed.
-const MIN_ZOOM = 0.5; +const MIN_ZOOM = 0.1; -const ZOOM_STEPS = [0.5, 1, 1.5, 2]; +const ZOOM_STEPS = [0.1, 0.25, 0.5, 1, 1.5, 2];
36-58: Add window resize handler for responsive behaviorThe initial magnification calculation considers viewport dimensions, but these dimensions could change if the user resizes their window. Consider adding a window resize handler to recalculate the optimal magnification.
useEffect(() => { if (!isFullScreenEnabled) return; + const handleResize = () => { + if (!imgRef.current) return; + const viewportWidth = window.innerWidth * 0.9; + const viewportHeight = window.innerHeight * 0.75; + const imageWidth = widthInNumber; + const imageHeight = imageWidth / aspectRatio; + + const widthRatio = viewportWidth / imageWidth; + const heightRatio = viewportHeight / imageHeight; + + setInitialMagnification(Math.min(widthRatio, heightRatio)); + }; + + window.addEventListener('resize', handleResize); + return () => window.removeEventListener('resize', handleResize); }, [isFullScreenEnabled, widthInNumber, aspectRatio]);
210-223: Add double-click to reset zoomConsider adding a double-click handler to reset the zoom level and position to initial state, which is a common feature in image viewers.
<img ref={setImageRef} src={src} className="read-only-image rounded-lg" + onDoubleClick={() => { + setMagnification(1); + if (imgRef.current) { + imgRef.current.style.left = "0px"; + imgRef.current.style.top = "0px"; + } + }} style={{ width: `${widthInNumber * initialMagnification}px`, maxWidth: "none", maxHeight: "none", aspectRatio, position: "relative", transform: `scale(${magnification})`, transformOrigin: "center", transition: "width 0.2s ease, transform 0.2s ease", }} onMouseDown={handleMouseDown} />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx(3 hunks)
🔇 Additional comments (1)
packages/editor/src/core/extensions/custom-image/components/toolbar/full-screen.tsx (1)
171-186: LGTM! Event listeners are properly managed
The event listeners are correctly set up with proper cleanup in the useEffect hook. The use of passive: false for the wheel event is appropriate for preventing default scroll behavior.
| const handleWheel = useCallback( | ||
| (e: WheelEvent) => { | ||
| if (!imgRef.current || !isFullScreenEnabled) return; | ||
|
|
||
| e.preventDefault(); | ||
|
|
||
| // Handle pinch-to-zoom | ||
| if (e.ctrlKey) { | ||
| const delta = e.deltaY; | ||
| setMagnification((prev) => { | ||
| const newZoom = prev * (1 - delta * ZOOM_SPEED); | ||
| const clampedZoom = Math.min(Math.max(newZoom, MIN_ZOOM), MAX_ZOOM); | ||
|
|
||
| // Reset position when zoom matches initial magnification | ||
| if (clampedZoom === 1 && imgRef.current) { | ||
| imgRef.current.style.left = "0px"; | ||
| imgRef.current.style.top = "0px"; | ||
| } | ||
|
|
||
| return clampedZoom; | ||
| }); | ||
| return; | ||
| } | ||
| }, | ||
| [handleClose] | ||
| [isFullScreenEnabled, magnification] | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix handleWheel implementation issues
Two issues need attention:
- The wheel zoom sensitivity might be too high for precise control
- The dependency array is missing dependencies used in the callback
-const ZOOM_SPEED = 0.05;
+const ZOOM_SPEED = 0.01;
const handleWheel = useCallback(
(e: WheelEvent) => {
// ... existing code ...
},
- [isFullScreenEnabled, magnification]
+ [isFullScreenEnabled]
);Committable suggestion skipped: line range outside the PR's diff.
| <div className="flex items-center"> | ||
| <button | ||
| type="button" | ||
| onClick={() => handleMagnification("decrease")} | ||
| className="size-6 grid place-items-center text-white/60 hover:text-white disabled:text-white/30 transition-colors duration-200" | ||
| disabled={magnification <= MIN_ZOOM} | ||
| > | ||
| <Minus className="size-4" /> | ||
| </button> | ||
| <span className="text-sm w-12 text-center text-white">{Math.round(100 * magnification)}%</span> | ||
| <button | ||
| type="button" | ||
| onClick={() => handleMagnification("increase")} | ||
| className="size-6 grid place-items-center text-white/60 hover:text-white disabled:text-white/30 transition-colors duration-200" | ||
| disabled={magnification >= MAX_ZOOM} | ||
| > | ||
| <Plus className="size-4" /> | ||
| </button> | ||
| </div> | ||
| <button |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility for zoom controls
The zoom controls need proper ARIA labels and roles for better accessibility.
<button
type="button"
onClick={() => handleMagnification("decrease")}
+ aria-label="Zoom out"
className="size-6 grid place-items-center text-white/60 hover:text-white disabled:text-white/30 transition-colors duration-200"
disabled={magnification <= MIN_ZOOM}
>
<Minus className="size-4" />
</button>
-<span className="text-sm w-12 text-center text-white">
+<span className="text-sm w-12 text-center text-white" role="status" aria-live="polite">
{Math.round(100 * magnification)}%
</span>
<button
type="button"
onClick={() => handleMagnification("increase")}
+ aria-label="Zoom in"
className="size-6 grid place-items-center text-white/60 hover:text-white disabled:text-white/30 transition-colors duration-200"
disabled={magnification >= MAX_ZOOM}
>
<Plus className="size-4" />
</button>📝 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.
| <div className="fixed bottom-10 left-1/2 -translate-x-1/2 flex items-center justify-center gap-1 rounded-md border border-white/20 py-2 divide-x divide-white/20 bg-black"> | |
| <div className="flex items-center"> | |
| <button | |
| type="button" | |
| onClick={() => handleMagnification("decrease")} | |
| className="size-6 grid place-items-center text-white/60 hover:text-white disabled:text-white/30 transition-colors duration-200" | |
| disabled={magnification <= MIN_ZOOM} | |
| > | |
| <Minus className="size-4" /> | |
| </button> | |
| <span className="text-sm w-12 text-center text-white">{Math.round(100 * magnification)}%</span> | |
| <button | |
| type="button" | |
| onClick={() => handleMagnification("increase")} | |
| className="size-6 grid place-items-center text-white/60 hover:text-white disabled:text-white/30 transition-colors duration-200" | |
| disabled={magnification >= MAX_ZOOM} | |
| > | |
| <Plus className="size-4" /> | |
| </button> | |
| </div> | |
| <div className="fixed bottom-10 left-1/2 -translate-x-1/2 flex items-center justify-center gap-1 rounded-md border border-white/20 py-2 divide-x divide-white/20 bg-black"> | |
| <div className="flex items-center"> | |
| <button | |
| type="button" | |
| onClick={() => handleMagnification("decrease")} | |
| aria-label="Zoom out" | |
| className="size-6 grid place-items-center text-white/60 hover:text-white disabled:text-white/30 transition-colors duration-200" | |
| disabled={magnification <= MIN_ZOOM} | |
| > | |
| <Minus className="size-4" /> | |
| </button> | |
| <span className="text-sm w-12 text-center text-white" role="status" aria-live="polite">{Math.round(100 * magnification)}%</span> | |
| <button | |
| type="button" | |
| onClick={() => handleMagnification("increase")} | |
| aria-label="Zoom in" | |
| className="size-6 grid place-items-center text-white/60 hover:text-white disabled:text-white/30 transition-colors duration-200" | |
| disabled={magnification >= MAX_ZOOM} | |
| > | |
| <Plus className="size-4" /> | |
| </button> | |
| </div> |
Palanikannan1437
changed the title
3 participants
Description
Adds in more zooming for small images, and also allows scroll zooming and moving around while dragging the image.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor