Add touch gestures to editor page - #226
Conversation
📝 WalkthroughWalkthroughAdds multi-pointer pinch-to-zoom and pan support to the Editor, updates CSS for touch/drag UX (grabbing cursor, disabled selection), adds tests for Editor and EditorHelmet, and introduces documentation for the Editor page and its tests. Changes
Sequence Diagram(s)(omitted — changes are client-side gesture handling and do not introduce a multi-component sequential flow suitable for a Mermaid diagram) Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Editor/Editor.module.css (1)
48-84: Includeellipsein interactive + preview selectors.
ellipseis supported in color mode and shape selector, but it’s missing from the.inner svg …and.previewMode …lists. That means ellipses can still toggle in preview mode and won’t get consistent hover styling.✅ Suggested fix
.inner svg path, .inner svg rect, .inner svg circle, .inner svg polygon, +.inner svg ellipse, .inner svg g { pointer-events: all; stroke: `#000`; stroke-width: 0.5; cursor: pointer; transition: stroke 80ms ease, stroke-width 80ms ease, opacity 80ms ease; opacity: 0.98; } .inner svg path:hover, .inner svg rect:hover, .inner svg circle:hover, .inner svg polygon:hover, +.inner svg ellipse:hover, .inner svg g:hover { stroke: var(--color-primary); stroke-width: 0.9; } .previewMode svg path, .previewMode svg rect, .previewMode svg circle, .previewMode svg polygon, +.previewMode svg ellipse, .previewMode svg g { pointer-events: none; }
🤖 Fix all issues with AI agents
In `@docs/docs/reference/pages/Editor/index.md`:
- Around line 113-117: The math block has a LaTeX typo using "ext{nextTx}"
instead of "\text{nextTx}"; update the formula to use \text{nextTx} and
\text{nextTy} so the expressions for nextTx and nextTy render correctly (i.e.,
fix the prefix to \text for both nextTx and nextTy in the equation involving
m_x, c_x, nextScale, m_y, c_y).
In `@src/pages/Editor/Editor.test.jsx`:
- Around line 20-25: The helper withElementFromPoint currently replaces
document.elementFromPoint but doesn't restore it if the async callback throws;
update withElementFromPoint to use a try/finally around the await fn() call so
that document.elementFromPoint is always reset to the original even when the
callback rejects or throws (keep references to the original variable and the
vi.fn mock as in the existing implementation).
In `@src/pages/Editor/index.jsx`:
- Around line 48-49: The file has stray blank lines causing Prettier warnings in
the Editor component; remove the extra empty lines (near the top of
src/pages/Editor/index.jsx and anywhere Prettier reports) so the component's
structure (e.g., the Editor function/component, its imports, hooks like
useEffect, and the export default) has single spacing, then run `prettier
--write` (or your project's formatting command) to reformat the file and commit
the changes.
- Around line 154-159: When reducing pointers to one (the
activePointersRef.current.size === 1 branch) avoid unconditionally setting
pointerState.current.moved = false because that causes a subsequent lift to
register as a new "tap" and toggle color after a pinch; instead check a pinch
flag (e.g., pointerState.current.isPinching or pointerState.current.pinchActive)
and only set moved = false if no pinch was active, otherwise preserve moved =
true and clear the pinch flag once the pinch gesture is fully finished; apply
this change in the same block that updates pointerState.current.lastX/lastY.
🧹 Nitpick comments (1)
src/pages/Editor/index.jsx (1)
33-34: Consider movingclampoutside the component.The
clampfunction is pure and doesn't depend on component state or props. Defining it inside the component causes unnecessary recreation on each render.Proposed refactor
+const clamp = (value, min, max) => Math.max(min, Math.min(max, value)); + // shape selector we care about const SHAPE_SELECTOR = 'path,rect,circle,polygon,ellipse'; export default function Editor() { // ... component code - const clamp = (value, min, max) => Math.max(min, Math.min(max, value));
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/docs/reference/pages/Editor/index.mddocs/docs/reference/pages/Editor/tests.mddocs/docs/reference/pages/index.mdsrc/pages/Editor/Editor.module.csssrc/pages/Editor/Editor.test.jsxsrc/pages/Editor/EditorHelmet.test.jsxsrc/pages/Editor/index.jsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T15:34:04.654Z
Learnt from: Ryan-Millard
Repo: Ryan-Millard/Img2Num PR: 146
File: docs/docs/reference/react/components/ThemeSwitch/tests.md:123-124
Timestamp: 2026-01-04T15:34:04.654Z
Learning: In Docusaurus documentation, relative directory links (e.g., ../ or ../../../hooks/useTheme) resolve to index.md within the target directory. Do not require or force linking to index.md explicitly; such relative links remain valid and should not be changed when reviewing docs under the docs directory.
Applied to files:
docs/docs/reference/pages/Editor/tests.mddocs/docs/reference/pages/Editor/index.mddocs/docs/reference/pages/index.md
🧬 Code graph analysis (3)
src/pages/Editor/Editor.test.jsx (1)
src/pages/Editor/index.jsx (1)
Editor(13-231)
src/pages/Editor/EditorHelmet.test.jsx (1)
src/pages/Editor/EditorHelmet.jsx (1)
EditorHelmet(4-11)
src/pages/Editor/index.jsx (1)
src/components/GlassCard.jsx (1)
GlassCard(5-9)
🪛 GitHub Actions: CI
src/pages/Editor/index.jsx
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
src/pages/Editor/Editor.module.css
[warning] 1-1: Code style issues found by Prettier. Run 'prettier --write' to fix.
🪛 LanguageTool
docs/docs/reference/pages/Editor/tests.md
[style] ~43-~43: To form a complete sentence, be sure to include a subject.
Context: ...l notes) - document.elementFromPoint can be stubbed to return the shape you want...
(MISSING_IT_THERE)
🔇 Additional comments (9)
docs/docs/reference/pages/index.md (1)
1-24: Solid pages overview and conventions.Clear structure and helpful guidance for route-level docs.
docs/docs/reference/pages/Editor/index.md (1)
8-112: Comprehensive and well-structured Editor page reference.The interaction and data-flow sections are clear and easy to follow.
src/pages/Editor/EditorHelmet.test.jsx (1)
1-33: Helmet assertions look solid.Good coverage of title, robots, description, and canonical link.
docs/docs/reference/pages/Editor/tests.md (1)
1-52: Helpful and practical testing guidance.The coverage list and event-testing notes are clear and actionable.
src/pages/Editor/Editor.test.jsx (2)
46-73: Good test coverage for color toggle behavior.The test correctly simulates a tap gesture using pointerDown/pointerUp events and validates the toggle mechanism through class assertion.
75-109: Thorough verification of preview mode behavior.The test properly validates the mode switch state, visual styling class application, and confirms color toggling is disabled in preview mode.
src/pages/Editor/index.jsx (3)
65-83: Well-implemented pinch-to-zoom initialization.The logic correctly captures the initial transform state and computes the content coordinate under the pinch midpoint for stable anchoring during zoom. Setting
moved = true(line 82) appropriately prevents accidental color toggles.
175-177: Good use ofclassList.togglefor color reveal.The toggle behavior correctly allows users to reveal and hide original colors repeatedly, improving the interaction model over the previous always-add approach.
217-218: Good addition ofonPointerCancelhandler.Reusing
onPointerUpfor cancellation events ensures proper cleanup when the browser interrupts pointer interactions (e.g., system gestures, incoming calls on mobile).
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| if (activePointersRef.current.size === 1) { | ||
| const remaining = Array.from(activePointersRef.current.values())[0]; | ||
| pointerState.current.lastX = remaining.x; | ||
| pointerState.current.lastY = remaining.y; | ||
| pointerState.current.moved = false; | ||
| } |
There was a problem hiding this comment.
Resetting moved after pinch may cause unintended color toggles.
When transitioning from two pointers to one, resetting moved = false (line 158) means lifting the remaining finger without movement will trigger a color toggle—even though the user just completed a pinch gesture. Consider preserving moved = true if a pinch was active.
Proposed fix
// If exactly one pointer remains, reset lastX/lastY so pan can continue smoothly
if (activePointersRef.current.size === 1) {
const remaining = Array.from(activePointersRef.current.values())[0];
pointerState.current.lastX = remaining.x;
pointerState.current.lastY = remaining.y;
- pointerState.current.moved = false;
+ // Keep moved=true if we just finished a pinch to prevent accidental tap
}🤖 Prompt for AI Agents
In `@src/pages/Editor/index.jsx` around lines 154 - 159, When reducing pointers to
one (the activePointersRef.current.size === 1 branch) avoid unconditionally
setting pointerState.current.moved = false because that causes a subsequent lift
to register as a new "tap" and toggle color after a pinch; instead check a pinch
flag (e.g., pointerState.current.isPinching or pointerState.current.pinchActive)
and only set moved = false if no pinch was active, otherwise preserve moved =
true and clear the pinch flag once the pinch gesture is fully finished; apply
this change in the same block that updates pointerState.current.lastX/lastY.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Editor/index.jsx (1)
139-172: Keep “grabbing” state until all pointers release.With multi-touch, a pointer-up can happen while another pointer is still down. Removing the grabbing class unconditionally drops the feedback even though dragging continues. Gate removal on
activePointersRef.current.size === 0.🩹 Suggested fix
- viewportRef.current?.classList.remove(styles.grabbing); + if (activePointersRef.current.size === 0) { + viewportRef.current?.classList.remove(styles.grabbing); + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/pages/Editor/Editor.test.jsxsrc/pages/Editor/index.jsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/pages/Editor/Editor.test.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/pages/Editor/index.jsx (1)
src/components/LoadingHedgehog.jsx (1)
clamp(9-9)
🔇 Additional comments (2)
src/pages/Editor/index.jsx (2)
21-118: Multi-pointer tracking & pinch anchoring look solid.Clean state bookkeeping and anchoring the pinch midpoint to content coordinates should keep zoom stable under the fingers.
218-220: Hint text matches the new toggle behavior.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is wonderful! Thank you!
Please will you move the documentation folder (docs/docs/reference/pages) to docs/docs/reference/react/pages because that's where all the react code docs belong for this project.
|
@CodeRabbit generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @Ryan-Millard. * #226 (comment) The following files were modified: * `src/pages/Editor/index.jsx`
|
@CodeRabbit, comment the docstrings instead of a PR in the future. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
|
|
@Ryan-Millard Thank you for the feedback , i have implemented the directions addressed by you. |
There was a problem hiding this comment.
This is great stuff! It works wonderfully on mobile.
I just noticed one small bug: tapping a region that has already been colored-in removes the color.
Have you been able to test this on your phone?
If you haven't been able to:
Run this to get your local IP address:ifconfig # for Unix shellsipconfig # for WindowsThen connect to
<your-local-IP-address>:5173/Img2Num/from your phone.Some routers may block this (some allow that to be changed, but some don't), so using mobile hotspot could be a good alternative if your router is like mine.
|
Additionally, it could be a nice clean move to separate the handlers into their own files - one for mobile and one for PC - and then importing them. That's optional, though. I don't mind too much. |
Yes i have tested that in mobile , adressing the removal of color on clicking the coloured region is set by me , i will disable that no worries. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Editor/index.jsx (1)
139-159: Keep the “grabbing” cursor while any pointer is still down.
classList.removeruns even if another pointer remains, so the cursor feedback can flicker during multi-touch. Consider removing the class only when all pointers are lifted.Proposed fix
- // Keep dragging true if another pointer remains down - pointerState.current.dragging = activePointersRef.current.size > 0; + // Keep dragging true if another pointer remains down + const hasPointers = activePointersRef.current.size > 0; + pointerState.current.dragging = hasPointers; @@ - viewportRef.current?.classList.remove(styles.grabbing); + if (!hasPointers) { + viewportRef.current?.classList.remove(styles.grabbing); + }
♻️ Duplicate comments (1)
src/pages/Editor/index.jsx (1)
1-9: Resolve the remaining Prettier warning before merge.CI still reports a formatting issue for this file; run the formatter and commit the result.
|
@Ryan-Millard I tried to keep the pointerEvent functions separate for desktop & touch based but it failed because we are using same eventListener ( eg. onPointerUp,onPointerMove etc ) for both desktop as well as for touch gestures so i kept it as it was , i have removed the color toggle which was removing color on clicking on colored regions. |
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is great!
Please will you implement the last changed I asked for, then it'll be good to go.
There was a problem hiding this comment.
This is quite a large file and the contents on the right sidebar go over the fold (see the image below). Please would you condense the contents of this file to make it faster to read through. It isn't a very complex piece of code, and new users will likely get frustrated by having to sift through large amounts of text to find what they're looking for.
There's nothing terribly wrong with your documentation, but this is a UX problem that will draw people away and possibly lead to poor quality PRs because they're too lazy to read. I've already encountered a fair amount of PRs like that.
There was a problem hiding this comment.
I have condensed the docs content and now it looks better
Ryan-Millard
changed the title
Ryan-Millard
changed the title
|
@Ryan-Millard Thank you for the appreciation, i understand the concern of large content in single file so i have condensed the content to keep it to the point to make it easy for others to understand. |
Ryan-Millard
left a comment
Ryan-Millard
left a comment
There was a problem hiding this comment.
This is wonderful. Thank you so much!
|
@codevory, please use the syntax below in the body of your future PRs to automatically close related issues upon merging: Fixes: #<issue-number> |
* added documentation for Editor * added css property to overide the default pointer events by browser * added tests for Editor in index.js * added tests for EditorHelmt * added touch gestures , pinch zoom , color toggle on click,image drag etc * Format Editor component spacing * addd error handling block * implemented the issues pointed out * improved the docs * improved the lint * improved docs structure & implemented feedback * disabled toggle color on click/tap * removed toggle color & refactored the docs * formatted structure * formatted with prettier * formatted with prettier * condensed the docs content to make it ease to read * fix(docs): broken editor & tests page links --------- Co-authored-by: Ryan Millard <142347829+Ryan-Millard@users.noreply.github.com>
|
@Ryan-Millard Alright i will keep it in mind for future PRs. |
Thank you! Also let me know if you'd like some help finding a nice issue. |
|
This broke the production code because GitHub Pages is a terrible hosting service.😢 I need to migrate elsewhere soon. Unfortunately, I'll only have time on Monday or Tuesday to do a migration. The problem is that we can't set our own headers, which causes the below: wasmWorker-9o6JCJ9n.js:1 Uncaught (in promise) DataCloneError: Failed to execute 'postMessage' on 'Worker': SharedArrayBuffer transfer requires self.crossOriginIsolated.
at wasmWorker-9o6JCJ9n.js:1:6126
at new Promise (<anonymous>)
at loadWasmModuleToWorker (wasmWorker-9o6JCJ9n.js:1:5380)
at Array.map (<anonymous>)
at Object.loadWasmModuleToAllWorkers (wasmWorker-9o6JCJ9n.js:1:6270)
at wasmWorker-9o6JCJ9n.js:1:4936
at We (wasmWorker-9o6JCJ9n.js:1:3849)
at fr (wasmWorker-9o6JCJ9n.js:1:2093)
at G (wasmWorker-9o6JCJ9n.js:1:13217)
at _e (wasmWorker-9o6JCJ9n.js:1:13448)
crossOriginIsolated
false
crossOriginIsolated=true
true
crossOriginIsolated
false |
Thanks for the clarification ,iam glad the root cause was identified. Happy to help where I can. |
I'm sorry. I commented this on the wrong PR by mistake. It was meant for #234. The problem is the addition of pthreads in @Krasner's refactor. |
Alright no worries |
Thanks, i have taken another issue & will be working on that but will be glad to meet those issues as well |
2 participants
Fixes: #202
This pull request introduces comprehensive documentation and testing for the Editor page, along with improvements to pointer/multi-touch interaction logic and CSS refinements for SVG shape handling , addressing the issue #202 . The main focus is to clarify the Editor's behavior, ensure reliable pan/zoom/tap interactions (including pinch-zoom), and provide clear guidance for contributors and future maintainers.
Key changes:
Documentation and Testing
docs/docs/reference/pages/Editor/index.mdand a dedicated test strategy indocs/docs/reference/pages/Editor/tests.md. A new index for page-level docs was also introduced. [1] [2] [3]src/pages/Editor/Editor.test.jsxand for metadata/SEO insrc/pages/Editor/EditorHelmet.test.jsx. [1] [2]Editor Page Interaction Logic
src/pages/Editor/index.jsxto support multi-touch pinch-zoom, with stable content anchoring under the pinch midpoint, and improved bookkeeping for active pointers and drag state. [1] [2] [3] [4].coloredRegionclass, and drag/pinch interactions do not trigger toggling.CSS and Styling Improvements
src/pages/Editor/Editor.module.cssto support all relevant SVG shapes for color reveal, improved pointer interaction feedback (including mobile), and clarified class responsibilities. [1] [2] [3]Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.