Uh oh!
There was an error while loading. Please reload this page.
feat(feedback): Revamp of user feedback screenshot editing - #15424
Merged
Conversation
Contributor
size-limit report 📦
|
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
Uh oh!
There was an error while loading. Please reload this page.
ryan953
reviewed
Feb 20, 2025
| border: var(--button-border, var(--border)); | ||
| border-radius: var(--button-border-radius, 6px); | ||
| background: var(--button-background, var(--background)); | ||
| color: var(--button-foreground, var(--foreground)); |
Member
There was a problem hiding this comment.
was this wrong before, not matching docs? now it's fixed?
ContributorAuthor
There was a problem hiding this comment.
I think when I added the annotation stuff, I mixed up the names and used the wrong one. This is technically not in the docs, but now it matches what's used in the form buttons
ryan953
reviewed
Feb 20, 2025
| .editor__tool--active { | ||
| background: var(--button-primary-background, var(--accent-background)); | ||
| color: var(--button-primary-foreground, var(--accent-foreground)); |
Member
There was a problem hiding this comment.
was this wrong before, not matching docs? now it's fixed?
ryan953
reviewed
Feb 20, 2025
Comment on lines
+99
to
+103
| if (sentryFeedback) { | ||
| strokeColor = | ||
| getComputedStyle(sentryFeedback).getPropertyValue('--button-primary-background') || | ||
| getComputedStyle(sentryFeedback).getPropertyValue('--accent-background'); | ||
| } |
Member
There was a problem hiding this comment.
Suggested change
| if(sentryFeedback){ | |
| strokeColor= | |
| getComputedStyle(sentryFeedback).getPropertyValue('--button-primary-background')|| | |
| getComputedStyle(sentryFeedback).getPropertyValue('--accent-background'); | |
| } | |
| if(sentryFeedback){ | |
| constcomputedStyle=getComputedStyle(sentryFeedback); | |
| strokeColor= | |
| computedStyle.getPropertyValue('--button-primary-background')|| | |
| computedStyle.getPropertyValue('--accent-background'); | |
| } |
ContributorAuthor
There was a problem hiding this comment.
also, not sure if this is the best way to do this, but it works
c298lee
marked this pull request as ready for review
February 21, 2025 18:36
billyvg
reviewed
Feb 21, 2025
billyvg
left a comment
Member
There was a problem hiding this comment.
mostly nits about naming/comments
Uh oh!
There was an error while loading. Please reload this page.
| <Annotations action={action} imageBuffer={imageBuffer} annotatingRef={annotatingRef} /> | ||
| <div class="editor__canvas-container" ref={measurementRef}> | ||
| <canvas ref={screenshotRef}></canvas> | ||
| <canvas class="editor__canvas-annotate" ref={graywashRef} onMouseDown={handleMouseDown}></canvas> |
Member
There was a problem hiding this comment.
It would be nice if the class names and ref name were more aligned. Also, graywash makes me think it's only used to draw the backdrop, but handleMouseDown looks like it's also handling the drawing of the boxes/rects. maybe this doesn't matter as much if we had some code comments about what the different refs are do.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| }; | ||
| const handleMouseUp = (e: MouseEvent): void => { | ||
| setCurrentRect(undefined); |
Member
There was a problem hiding this comment.
can you add a comment why we're setting current rect as undefined?
| } | ||
| }, [currentRect]); | ||
| const drawBuffer = hooks.useCallback((): void => { |
Uh oh!
There was an error while loading. Please reload this page.
Comment on lines
+136
to
+144
| const [drawCommands, setDrawCommands] = hooks.useState<Rect[]>([]); | ||
| const [currentRect, setCurrentRect] = hooks.useState<Rect | undefined>(undefined); | ||
| const measurementRef = hooks.useRef<HTMLDivElement>(null); | ||
| const screenshotRef = hooks.useRef<HTMLCanvasElement>(null); | ||
| const graywashRef = hooks.useRef<HTMLCanvasElement>(null); | ||
| const rectDivRef = hooks.useRef<HTMLDivElement>(null); | ||
| const [imageSource, setimageSource] = hooks.useState<HTMLCanvasElement | null>(null); | ||
| const [displayEditor, setdisplayEditor] = hooks.useState<boolean>(true); | ||
| const [scaleFactor, setScaleFactor] = hooks.useState<number>(1); |
Member
There was a problem hiding this comment.
It might help to have comments explaining the purposes these (where it makes sense)
e.g. what is the purpose of the graywashRef
| // draws outline around rectangle in the same colour as the submit button | ||
| let strokeColor; | ||
| const sentryFeedback = DOCUMENT.getElementById('sentry-feedback'); |
Member
There was a problem hiding this comment.
is sentry-feedback id always guaranteed to exist?
ContributorAuthor
There was a problem hiding this comment.
It's the id attribute of the div that contains the feedback widget. Good catch tho, this can actually be customized
ryan953
changed the base branch from
develop
to
ryan953/feedback-highlight-maskFebruary 24, 2025 23:22
ryan953 pushed a commit
that referenced
this pull request
Feb 24, 2025
Allows you to edit screenshots with highlight and hide boxes, with the ability to delete individual boxes and resize the page. This will replace the previous cropping and pen annotation.  Related to getsentry/sentry#69786
billyvg pushed a commit
that referenced
this pull request
Feb 25, 2025
Allows you to edit screenshots with highlight and hide boxes, with the ability to delete individual boxes and resize the page. This will replace the previous cropping and pen annotation.  Related to getsentry/sentry#69786
ryan953 added a commit
that referenced
this pull request
Apr 3, 2025
…shots (#15951) | Editing | In Sentry | | --- | --- | | <img width="1408" alt="SCR-20250402-pfuj" src="https://github.com/user-attachments/assets/896a1efe-74ac-42ca-8f51-b7024dee768c" /> | <img width="1421" alt="SCR-20250402-pgft" src="https://github.com/user-attachments/assets/37337f95-9e4d-477f-935b-2545466bc455" /> I should write that there are like 3 separate systems inside the `ScreenshotEditor` component, all working with the same data and helpers really, but each easy to spot and understand on their own. The end-goal is to have the correct thing rendered into `outputBuffer` (which is an argument to the component, but it's used for outputting data) and also render into the `foreground` and `background`. 1. The first is the `useLayoutEffect` and `useEffect` calls. These basically depend on the `screenshot` prop & `drawCommands` state. When those change we're basically re-sizing and re-rendering into our canvases. 2. The second system is all the stuff inside of `handleMouseDown`. We call `getBoundingClientRect` and use offsetX/offsetY for a consistent frame if reference when doing mouse coordinates. From in here we don't update any react state until onMouseUp happens, so the re-rendering onMouseMove is really fast to execute, and really easy to understand. 3. The last system to look at is is the `drawCommands` stuff and `scaleFactor`. All the drawCommands in state/memory are using an x/y coordinate system based on the size of the original screenshot. So when we call paintForeground it's really easy. But when we want to add `<div>` elements to the page we have to scale the sizes. This helps with window resize, and helps to consolidate when/where we convert from one x/y coordinate system to another. The resulting feedbacks are in the correct dpi, and have boxes draw using the same DPI. | Case | Img | | --- | --- | | No boxes drawn ever | <img width="485" alt="SCR-20250402-jidz" src="https://github.com/user-attachments/assets/3f5d177a-4652-40c1-be89-bfa2ff4f7399" /> | | Drew some, then removed them | <img width="452" alt="SCR-20250402-jifp" src="https://github.com/user-attachments/assets/c24fc144-d0f1-4b64-af6c-199eff0a3fee" /> | | Some boxes | <img width="452" alt="SCR-20250402-jigm" src="https://github.com/user-attachments/assets/a3af82b1-16a8-4a78-bac4-5d83f687a59f" /> | Related to #15424 Iteration on top of develop...ryan953/feedback-highlight-mask-refFixesgetsentry/sentry#69786 --------- Co-authored-by: Billy Vong <billyvg@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
DO NOT MERGE
Allows you to edit screenshots with highlight and hide boxes, with the ability to delete individual boxes and resize the page. This will replace the previous cropping and pen annotation.
Closesgetsentry/sentry#69786