From 78931ff945851cde3a27f1d49d18506ab3ed807e Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Sat, 16 Mar 2024 03:25:48 -0400 Subject: [PATCH 1/3] fix cropping jank & cursor --- .../components/ScreenshotEditor.tsx | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx b/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx index d3753631ec1c..7230e0d00498 100644 --- a/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx +++ b/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx @@ -1,4 +1,4 @@ -// eslint-disable max-lines +/* eslint-disable max-lines */ import type { ComponentType, VNode, h as hType } from 'preact'; // biome-ignore lint: needed for preact import { h } from 'preact'; // eslint-disable-line @typescript-eslint/no-unused-vars @@ -118,44 +118,46 @@ export function makeScreenshotEditorComponent({ h, imageBuffer, dialog }: Factor setConfirmCrop(false); const handleMouseMove = makeHandleMouseMove(corner); const handleMouseUp = (): void => { - croppingRef.current && croppingRef.current.removeEventListener('mousemove', handleMouseMove); + DOCUMENT.removeEventListener('mousemove', handleMouseMove); DOCUMENT.removeEventListener('mouseup', handleMouseUp); setConfirmCrop(true); }; DOCUMENT.addEventListener('mouseup', handleMouseUp); - croppingRef.current && croppingRef.current.addEventListener('mousemove', handleMouseMove); + DOCUMENT.addEventListener('mousemove', handleMouseMove); } const makeHandleMouseMove = useCallback((corner: string) => { return function (e: MouseEvent) { + const mouseX = e.clientX - croppingRef.current!.getBoundingClientRect().x; + const mouseY = e.clientY - croppingRef.current!.getBoundingClientRect().y; switch (corner) { case 'topleft': setCroppingRect(prev => ({ ...prev, - startx: Math.min(e.offsetX, prev.endx - 30), - starty: Math.min(e.offsetY, prev.endy - 30), + startx: Math.min(Math.max(0, mouseX), prev.endx - 30), + starty: Math.min(Math.max(0, mouseY), prev.endy - 30), })); break; case 'topright': setCroppingRect(prev => ({ ...prev, - endx: Math.max(e.offsetX, prev.startx + 30), - starty: Math.min(e.offsetY, prev.endy - 30), + endx: Math.max(Math.min(mouseX, croppingRef.current!.width), prev.startx + 30), + starty: Math.min(Math.max(0, mouseY), prev.endy - 30), })); break; case 'bottomleft': setCroppingRect(prev => ({ ...prev, - startx: Math.min(e.offsetX, prev.endx - 30), - endy: Math.max(e.offsetY, prev.starty + 30), + startx: Math.min(Math.max(0, mouseX), prev.endx - 30), + endy: Math.max(Math.min(mouseY, croppingRef.current!.height), prev.starty + 30), })); break; case 'bottomright': setCroppingRect(prev => ({ ...prev, - endx: Math.max(e.offsetX, prev.startx + 30), - endy: Math.max(e.offsetY, prev.starty + 30), + endx: Math.max(Math.min(mouseX, croppingRef.current!.width), prev.startx + 30), + endy: Math.max(Math.min(mouseY, croppingRef.current!.height), prev.starty + 30), })); break; } @@ -317,6 +319,7 @@ function CropCorner({ borderRight: corner === 'topright' || corner === 'bottomright' ? 'solid purple' : 'none', borderBottom: corner === 'bottomleft' || corner === 'bottomright' ? 'solid purple' : 'none', borderWidth: '3px', + cursor: corner === 'topleft' || corner === 'bottomright' ? 'nwse-resize' : 'nesw-resize', }} onMouseDown={e => { e.preventDefault(); From c6a4764e8a65dddb51ea4c9a96632892569aae49 Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Sat, 16 Mar 2024 18:34:38 -0400 Subject: [PATCH 2/3] fix smallest crop & lint --- .../components/ScreenshotEditor.tsx | 24 +++++++++++-------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx b/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx index 7230e0d00498..095149f158ef 100644 --- a/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx +++ b/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx @@ -129,35 +129,39 @@ export function makeScreenshotEditorComponent({ h, imageBuffer, dialog }: Factor const makeHandleMouseMove = useCallback((corner: string) => { return function (e: MouseEvent) { - const mouseX = e.clientX - croppingRef.current!.getBoundingClientRect().x; - const mouseY = e.clientY - croppingRef.current!.getBoundingClientRect().y; + if (!croppingRef.current) { + return; + } + const cropCanvas = croppingRef.current; + const mouseX = e.clientX - cropCanvas.getBoundingClientRect().x; + const mouseY = e.clientY - cropCanvas.getBoundingClientRect().y; switch (corner) { case 'topleft': setCroppingRect(prev => ({ ...prev, - startx: Math.min(Math.max(0, mouseX), prev.endx - 30), - starty: Math.min(Math.max(0, mouseY), prev.endy - 30), + startx: Math.min(Math.max(0, mouseX), prev.endx - 33), + starty: Math.min(Math.max(0, mouseY), prev.endy - 33), })); break; case 'topright': setCroppingRect(prev => ({ ...prev, - endx: Math.max(Math.min(mouseX, croppingRef.current!.width), prev.startx + 30), - starty: Math.min(Math.max(0, mouseY), prev.endy - 30), + endx: Math.max(Math.min(mouseX, cropCanvas.width), prev.startx + 33), + starty: Math.min(Math.max(0, mouseY), prev.endy - 33), })); break; case 'bottomleft': setCroppingRect(prev => ({ ...prev, - startx: Math.min(Math.max(0, mouseX), prev.endx - 30), - endy: Math.max(Math.min(mouseY, croppingRef.current!.height), prev.starty + 30), + startx: Math.min(Math.max(0, mouseX), prev.endx - 33), + endy: Math.max(Math.min(mouseY, cropCanvas.height), prev.starty + 33), })); break; case 'bottomright': setCroppingRect(prev => ({ ...prev, - endx: Math.max(Math.min(mouseX, croppingRef.current!.width), prev.startx + 30), - endy: Math.max(Math.min(mouseY, croppingRef.current!.height), prev.starty + 30), + endx: Math.max(Math.min(mouseX, cropCanvas.width), prev.startx + 33), + endy: Math.max(Math.min(mouseY, cropCanvas.height), prev.starty + 33), })); break; } From 691eaacfa094b62c80c415f9c65aadba91014544 Mon Sep 17 00:00:00 2001 From: Catherine Lee <55311782+c298lee@users.noreply.github.com> Date: Mon, 18 Mar 2024 14:51:58 -0400 Subject: [PATCH 3/3] PR comments --- .../components/ScreenshotEditor.tsx | 77 ++++++++++--------- 1 file changed, 41 insertions(+), 36 deletions(-) diff --git a/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx b/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx index 095149f158ef..a698f388b7c0 100644 --- a/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx +++ b/packages/feedback/src/screenshot/components/ScreenshotEditor.tsx @@ -8,6 +8,10 @@ import type { Dialog } from '../../types'; import { createScreenshotInputStyles } from './ScreenshotInput.css'; import { useTakeScreenshot } from './useTakeScreenshot'; +const CROP_BUTTON_SIZE = 30; +const CROP_BUTTON_BORDER = 3; +const CROP_BUTTON_OFFSET = CROP_BUTTON_SIZE + CROP_BUTTON_BORDER; + interface FactoryParams { h: typeof hType; imageBuffer: HTMLCanvasElement; @@ -19,10 +23,10 @@ interface Props { } interface Box { - startx: number; - starty: number; - endx: number; - endy: number; + startX: number; + startY: number; + endX: number; + endY: number; } interface Rect { @@ -34,10 +38,10 @@ interface Rect { const constructRect = (box: Box): Rect => { return { - x: Math.min(box.startx, box.endx), - y: Math.min(box.starty, box.endy), - width: Math.abs(box.startx - box.endx), - height: Math.abs(box.starty - box.endy), + x: Math.min(box.startX, box.endX), + y: Math.min(box.startY, box.endY), + width: Math.abs(box.startX - box.endX), + height: Math.abs(box.startY - box.endY), }; }; @@ -51,7 +55,7 @@ const getContainedSize = (img: HTMLCanvasElement): Box => { } const x = (img.clientWidth - width) / 2; const y = (img.clientHeight - height) / 2; - return { startx: x, starty: y, endx: width + x, endy: height + y }; + return { startX: x, startY: y, endX: width + x, endY: height + y }; }; // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -62,7 +66,7 @@ export function makeScreenshotEditorComponent({ h, imageBuffer, dialog }: Factor const canvasContainerRef = useRef(null); const cropContainerRef = useRef(null); const croppingRef = useRef(null); - const [croppingRect, setCroppingRect] = useState({ startx: 0, starty: 0, endx: 0, endy: 0 }); + const [croppingRect, setCroppingRect] = useState({ startX: 0, startY: 0, endX: 0, endY: 0 }); const [confirmCrop, setConfirmCrop] = useState(false); useEffect(() => { @@ -85,7 +89,7 @@ export function makeScreenshotEditorComponent({ h, imageBuffer, dialog }: Factor cropButton.style.top = `${imageDimensions.y}px`; } - setCroppingRect({ startx: 0, starty: 0, endx: imageDimensions.width, endy: imageDimensions.height }); + setCroppingRect({ startX: 0, startY: 0, endX: imageDimensions.width, endY: imageDimensions.height }); } useEffect(() => { @@ -133,35 +137,36 @@ export function makeScreenshotEditorComponent({ h, imageBuffer, dialog }: Factor return; } const cropCanvas = croppingRef.current; - const mouseX = e.clientX - cropCanvas.getBoundingClientRect().x; - const mouseY = e.clientY - cropCanvas.getBoundingClientRect().y; + const cropBoundingRect = cropCanvas.getBoundingClientRect(); + const mouseX = e.clientX - cropBoundingRect.x; + const mouseY = e.clientY - cropBoundingRect.y; switch (corner) { case 'topleft': setCroppingRect(prev => ({ ...prev, - startx: Math.min(Math.max(0, mouseX), prev.endx - 33), - starty: Math.min(Math.max(0, mouseY), prev.endy - 33), + startX: Math.min(Math.max(0, mouseX), prev.endX - CROP_BUTTON_OFFSET), + startY: Math.min(Math.max(0, mouseY), prev.endY - CROP_BUTTON_OFFSET), })); break; case 'topright': setCroppingRect(prev => ({ ...prev, - endx: Math.max(Math.min(mouseX, cropCanvas.width), prev.startx + 33), - starty: Math.min(Math.max(0, mouseY), prev.endy - 33), + endX: Math.max(Math.min(mouseX, cropCanvas.width), prev.startX + CROP_BUTTON_OFFSET), + startY: Math.min(Math.max(0, mouseY), prev.endY - CROP_BUTTON_OFFSET), })); break; case 'bottomleft': setCroppingRect(prev => ({ ...prev, - startx: Math.min(Math.max(0, mouseX), prev.endx - 33), - endy: Math.max(Math.min(mouseY, cropCanvas.height), prev.starty + 33), + startX: Math.min(Math.max(0, mouseX), prev.endX - CROP_BUTTON_OFFSET), + endY: Math.max(Math.min(mouseY, cropCanvas.height), prev.startY + CROP_BUTTON_OFFSET), })); break; case 'bottomright': setCroppingRect(prev => ({ ...prev, - endx: Math.max(Math.min(mouseX, cropCanvas.width), prev.startx + 33), - endy: Math.max(Math.min(mouseY, cropCanvas.height), prev.starty + 33), + endX: Math.max(Math.min(mouseX, cropCanvas.width), prev.startX + CROP_BUTTON_OFFSET), + endY: Math.max(Math.min(mouseY, cropCanvas.height), prev.startY + CROP_BUTTON_OFFSET), })); break; } @@ -235,33 +240,33 @@ export function makeScreenshotEditorComponent({ h, imageBuffer, dialog }: Factor
{