-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(feedback): Refactor the feedback screenshot rendering to show N screenshots side by side #68136
Conversation
…screenshots side by side
cc @Jesse-Box I don't love the trash cans when there is more than 1 image (it's possible to submit more than one to the api, not via the feedback widget though). super easy to update the delete button, but low-low pri. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good! curious if it'd look better or worse if the background surrounding the screenshots was transparent rather than white
organization: Organization; | ||
projectSlug: Project['slug']; | ||
screenshot: EventAttachment; | ||
screenshotInFocus: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[event, organization.slug, projectSlug, screenshotInFocus, screenshots] | ||
); | ||
|
||
if (!hasContext && !screenshots.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we don't need any of this hasContext
stuff? :0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i took it out because i think we really only care that there are screenshots. Within the rest of the page we don't show context at all, we get the message from feedbackItem.metadata.message
for example. So I think if it's there or not, it shouldn't block our images.
I'm a bit worried that we're assuming the attachments will be images though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we validate somehow that they are images?
@michellewzhang I was thinking that too, this was calling for me: https://sentry.sentry.io/stories/?name=app/components/container/negativeSpaceContainer.stories.tsx But i didn't play with it because the space around only appears when there are multiple images at different dimensions. I think that's going to be really rare. |
…screenshots side by side (#68136) Related to #67707 Here are some examples of one or more attached screenshots, with different sizes: | Desc | Img | | --- | --- | | single small | ![single small](https://github.com/getsentry/sentry/assets/187460/9b4051e5-0a4e-4c58-bf9d-4e083b7e3cbe) | two same size | ![double same size](https://github.com/getsentry/sentry/assets/187460/e09a7ac7-cc4a-431a-94eb-39a02f31e3ae) | many wrapped | ![many wrapped](https://github.com/getsentry/sentry/assets/187460/14e9c930-093c-49f2-9565-3f602f196750) | many inline | ![many inline](https://github.com/getsentry/sentry/assets/187460/0db6c0f3-1a27-40cf-9e12-608123d2449f)
Related to #67707
Here are some examples of one or more attached screenshots, with different sizes: