Fix mobile tap behavior on Unsplash images #24586
Conversation
|
""" WalkthroughThe Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (1)
ghost/admin/app/components/gh-unsplash-photo.js (1)
58-70: Excellent implementation of touch-specific zoom behavior!The logic correctly addresses the PR objectives by:
- Allowing overlay button interactions on touch devices (lines 62-64)
- Disabling zoom for all other touch interactions (lines 66-70)
- Preserving existing desktop behavior
The use of
closest()to detect overlay button clicks is appropriate and the event handling is correct.Consider extracting the overlay button selectors to constants for better maintainability:
+ // Overlay button selectors for touch device handling + static OVERLAY_BUTTON_SELECTORS = [ + '.gh-unsplash-button-likes', + '.gh-unsplash-button-download', + '.gh-unsplash-photo-author' + ]; + @action zoom(event) { const {target} = event; - const isOverlayButtonClick = target.closest('.gh-unsplash-button-likes') || - target.closest('.gh-unsplash-button-download') || - target.closest('.gh-unsplash-photo-author'); + const isOverlayButtonClick = GhUnsplashPhoto.OVERLAY_BUTTON_SELECTORS + .some(selector => target.closest(selector));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ghost/admin/app/components/gh-unsplash-photo.js(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Admin tests - Chrome
- GitHub Check: E2E tests
🔇 Additional comments (1)
ghost/admin/app/components/gh-unsplash-photo.js (1)
44-46: LGTM! Touch device detection implementation is correct.The use of
(pointer: coarse)media query is the standard and reliable way to detect touch devices. The getter pattern ensures reactive evaluation when accessed.
|
Hi @niranjan-uma-shankar thanks for all your bug reports and fixes 🙏 |
|
Noted, I'll hold off as you suggested. Thank you for informing :) |
@ErisDS I implemented a couple more PRs to fix some bugs but kept them in draft mode specifically to avoid adding to the active queue. If it’s better to avoid opening even drafts for now, I can close them and re-open later. Let me know what works best for the team, thanks! |
|
@niranjan-uma-shankar I haven't fully reviewed yet but the premise of the change sounds great! We have two different Unsplash selector components currently, with a different React based component used for images inside the editor and the publication cover image in settings area, do you see a similar problem with mobile/tablet usage there? |
Zooming on Unsplash images in mobile and tablet devices leads to an awkward UI, not compatible with small screens. This removes the zoom capability on mobile and tablet devices.
Overlay button clicks are allowd to pass through on mobile devices
Ref TryGhost#24585 When the browser window on a desktop is resized to a small viewport, this fix ensures that the zoom behaviour is disabled. Zoom continues to work in tab viewports in non-touch devices.
niranjan-uma-shankar
force-pushed
the
fix-mobile-unsplash-zoom
branch
from
5cc7160 to
8ce814b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ghost/admin/app/styles/components/unsplash.css (1)
134-139: Broaden targeting to touch devices to avoid cursor/behavior mismatch on tablets and touch laptopsWidth-only targeting can still show a zoom-in cursor on larger touch devices where JS disables zoom. Include a pointer-capability query so the visual affordance matches behavior on touch.
-@media (max-width: 540px) { +@media (max-width: 540px), (hover: none) and (pointer: coarse) { .gh-unsplash-photo { cursor: default; } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ghost/admin/app/components/gh-unsplash-photo.js(2 hunks)ghost/admin/app/styles/components/unsplash.css(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ghost/admin/app/components/gh-unsplash-photo.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Setup
🔇 Additional comments (2)
ghost/admin/app/styles/components/unsplash.css (2)
134-139: Specificity check: safe against zoom overlay stylesThe new rule won’t override
.gh-unsplash-zoom .gh-unsplash-photo { cursor: zoom-out; }due to lower specificity, which is correct. Good addition to align visuals with the updated interaction model.
134-139: Confirm 540px breakpoint aligns with existing breakpoints
I scanned all@media (max-width: …px)usages across the codebase and found common values at 300px, 400px, 440–460px, 500px, 520px, 550px and 600px—but 540px appears only in this component. If our design system’s “mobile” cutoff is 576px (or 550px), we should update it; otherwise please confirm that 540px is the intended threshold.• ghost/admin/app/styles/components/unsplash.css: lines 134–139
@kevinansfield Thanks for bringing this up. I reviewed the behaviour of the React-based Unsplash modal, which is used in the lexical editor and in "Publication cover" modal in
Note:My recent PRs are functionally complete but currently in draft mode, per this request to avoid overwhelming the review queue. I’m happy to mark them “Ready for review” as soon as I get the go-ahead. |
|
@niranjan-uma-shankar feel free to move those PRs into ready-to-review, we're out of the 6.0 code freeze period now 🙂 |
|
Hi @kevinansfield 👋 just circling back on this one. It would be great to get some eyes on this PR, along with the other related Unsplash search ones:
Happy to make any adjustments if needed, just let me know! |
3 participants
Summary
This PR disables the zoom-on-tap behavior for Unsplash images in mobile viewports. Instead of zooming (which doesn't work well on mobile viewports), tapping now reveals the action buttons (e.g. “Insert” and “❤️”), making the interaction more intuitive for touch users.
Why this change?
This fixes #24585.
Behaviour before
Tapping an image:
Screen.Recording.2025-08-02.at.6.51.29.PM.mov
tbbf1.mov
Behaviour after
This PR handles touch devices and non-touch browser resized windows correctly.
In touch-enabled devices, tapping an image:
aft1.mov
Screen.Recording.2025-08-02.at.8.02.15.PM.mov
In non-touch devices, for example a desktop browser resized to a small viewport:
res1.mp4
Testing instructions
/posts, create a new post and click on "Add a feature image".