feat(feedback): Update user feedback screenshot and cropping to align with designs #11227
Conversation
This file contains 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
size-limit report 📦
|
billyvg
reviewed
Mar 21, 2024
packages/feedback/src/screenshot/components/ScreenshotEditor.tsx
Outdated
Show resolved
Hide resolved
packages/feedback/src/screenshot/components/ScreenshotEditor.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
36
to
37
|
|
||
| --crop-foreground: ${theme.cropForeground}; |
There was a problem hiding this comment.
imo, we don't allow theming of this button and use submitButton theme instead. This forces the UI to be a bit more consistent. I think it's ok to keep the CSS variable.
Suggested change
| --crop-foreground: ${theme.cropForeground}; | |
| --crop-foreground: ${theme.submitForeground}; |
There was a problem hiding this comment.
the crop "buttons" I'm referring to are the 4 corners that you can drag to crop, I'll rename them so it's more obvious
There was a problem hiding this comment.
Ah can you post a screenshot in the PR and maybe consult with @Jesse-Box? I think similar idea applies even though this isn't strictly a button, we want to maintain some sort of uniform theming so there aren't too many one-off colors.
billyvg
approved these changes
Mar 21, 2024
Recommend reading through https://web.dev/articles/ttfb before review. In https://www.notion.so/sentry/TTFB-vital-is-0-for-navigation-events-2337114dd75542569eb70255a467aba6 we identified that ttfb was being incorrectly calculated in certain scenarios. This was because we were calculating `ttfb` relative to transaction start time, **before** it has been adjusted by `browser` related spans about request/response ( remember browser tracing adjusts the start timestamp of a pageload transaction after adding certain request/response related spans). This meant that `Math.max(responseStartTimestamp - transactionStartTime, 0)` would just end up being `0` most of the time because using `transactionStartTime` was not correct. To fix this, we avoid trying to rely on our `transactionStartTime` timestamp at all, but instead using the web vitals version helper for this. When this gets merged in, I'll backport it to v7. I'm doing this in v8 first because I don't want to deal with the merge conflict that comes when we eventually migrate this code from tracing internal into the browser package. As a next step, we should seriously think about getting rid of all of our vendored code and just rely on the web vitals library - it's a huge pain to maintain this, and I'm sure there are some insidious bugs sneaking about.
Fixes issue where screenshotting in Safari results in black bars on the right and bottom of the image. Before: <img width="1278" alt="image" src="https://github.com/getsentry/sentry-javascript/assets/55311782/b2c174ad-8403-459a-b24b-7e055da3d657"> After: <img width="1278" alt="image" src="https://github.com/getsentry/sentry-javascript/assets/55311782/85fb850d-6c01-48cd-9fe0-21a860114b9d"> Relates to getsentry/sentry#63749
…rent request (#11130) When switching the SvelteKit server side SDK to `@sentry/node` powered by Otel, the semantics behind `continueTrace` changed as outlined in #11199. TLDR: We previously called `continueTrace` in a nested way when dealing with sub-requests`*` in SvelteKit. In our old Node SDK, this did nothing; in the new SDK, this currently causes a new root span/transaction to be created for the sub request. This patch now ensures that we continue to send sub request spans as child spans of the top-level/parent request.
The debugger call stack does not include the `new Promise` frames that are parsed from `error.stack`. This means that when we go through the frames to apply the local variables, the frames don't match up and we bail. This patch ignores those frames when matching functions in the frames.
This functionality is no longer being used, so we can remove it and it's tests. Part of #9885 since it makes it easier to port over browsertracing into browser package.
Make `integration.setupOnce` accept no arguments. This will allow us to easily remove `addGlobalEventProcessor` which is deprecated API. This also means we can remove `IntegrationFnResult`, as the type signature of the functional and class based integrations are now the same. Next up - remove `addGlobalEventProcessor`!
Feedback from Bruno. Also went ahead and updated https://github.com/getsentry/sentry-javascript/releases/tag/8.0.0-alpha.5 accordingly.
c298lee
changed the title
c298lee
commented
Mar 22, 2024
Comment on lines
+83
to
+90
| cropper.width = imageDimensions.width * DPI; | ||
| cropper.height = imageDimensions.height * DPI; | ||
| cropper.style.width = `${imageDimensions.width}px`; | ||
| cropper.style.height = `${imageDimensions.height}px`; | ||
| const ctx = cropper.getContext('2d'); | ||
| if (ctx) { | ||
| ctx.scale(DPI, DPI); | ||
| } |
There was a problem hiding this comment.
This reduces the crop rectangle blurriness, which is a known issue on retina screens: https://developer.mozilla.org/en-US/docs/Web/API/Window/devicePixelRatio
Bundle ReportChanges will decrease total bundle size by 2.81MB ⬇️
|
billyvg
reviewed
Mar 26, 2024
| width: 30px; | ||
| height: 30px; | ||
| position: absolute; | ||
| background: none; | ||
| border: solid #FFFFFF; |
There was a problem hiding this comment.
Can we add this to constants and share it with the canvas style?
billyvg
approved these changes
Mar 26, 2024
ryan953
approved these changes
Mar 26, 2024
cadesalaberry
pushed a commit
to cadesalaberry/sentry-javascript
that referenced
this pull request
Apr 19, 2024
… with designs (getsentry#11227) Updated some screenshot styles to align with design: - Changed cropping to more closely follow photoshop: darker background, white cropping corners + rectangle and additional black border for contrast if the screenshot is white - Changed naming to follow BES naming - Added padding so that the submit/cancel buttons are always visible without scrolling - Reduced blurriness of cropping rectangle <img width="1270" alt="image" src="https://github.com/getsentry/sentry-javascript/assets/55311782/df73a537-58f9-4f03-9edc-403039e61122"> Relates to getsentry/sentry#63749 ---------
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Updated some screenshot styles to align with design:
Relates to getsentry/sentry#63749