Skip to content
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

Update screenshots in Feedback Details to align with designs #67707

Closed
Tracked by #63749
c298lee opened this issue Mar 26, 2024 · 2 comments
Closed
Tracked by #63749

Update screenshots in Feedback Details to align with designs #67707

c298lee opened this issue Mar 26, 2024 · 2 comments
Assignees

Comments

@c298lee
Copy link
Member

c298lee commented Mar 26, 2024

Designs here: https://www.figma.com/file/hIUACYgu3lenuAPLgg22a3/Specs%3A-Screenshots-v1?type=design&node-id=29-1698&mode=design&t=eXQl14ol7pADOffQ-0

Currently screenshots in feedback details just reuse the screenshot preview from issue details

@c298lee c298lee changed the title Update screenshots in Feedback Details to align with designs: https://www.figma.com/file/hIUACYgu3lenuAPLgg22a3/Specs%3A-Screenshots-v1?type=design&node-id=29-1698&mode=design&t=eXQl14ol7pADOffQ-0 Update screenshots in Feedback Details to align with designs Mar 26, 2024
michellewzhang added a commit that referenced this issue Mar 27, 2024
hook will be modified as needed, but a starting point to make it easier
to implement core functionality of rendering the screenshots while using
our own UI (for #67707)
@michellewzhang
Copy link
Member

michellewzhang commented Mar 29, 2024

TODOS

  • Currently, if you delete a screenshot, it will not show up in UI anymore. The design specifies that we should instead show this message instead:
SCR-20240329-kuoq
  • Pagination code largely copied from the issues component; not tested since we can't currently add multiple screenshots in a FB submission yet
  • Screenshot display in details isn't scaling properly -- if a screenshot comes in that's extra wide or extra long (e.g. more than 360px in either dimension, it'll get cropped to fit within 360px x 360px. However the design specifies that the entire image should instead scale down to fit within this box, rather than getting cropped
  • When clicking into full screen/zoom-in mode, the modal should take up the whole screen rather than staying the same dimensions as the image. (see designs for reference)

michellewzhang added a commit that referenced this issue Apr 1, 2024
- Relates to #67707
- Implements some of the design updates from this
[figma](#67707) (see followup
todos at the bottom)
- Updates screenshot display in feedback UI to be part of the purple
message section. Removes extraneous buttons and adds a small delete
button to the right
- Removes extraneous buttons in full screen modal view

## Max dimensions are currently `360px` x `360px`:
<img width="743" alt="SCR-20240329-kqdj"
src="https://github.com/getsentry/sentry/assets/56095982/ba45f3c8-0d21-4824-a3ca-0215bdbb58ae">
<img width="829" alt="SCR-20240329-kqak"
src="https://github.com/getsentry/sentry/assets/56095982/007b9040-d756-4807-98c0-e747a973f848">
<img width="819" alt="SCR-20240329-kpyc"
src="https://github.com/getsentry/sentry/assets/56095982/94a5f111-2618-4798-9b70-73a35bc4022b">

## Full screen / zoom view 
- scales nicely
- zoom-in pointer on hover


https://github.com/getsentry/sentry/assets/56095982/3c97fa45-2109-4f19-9a71-d8e8084397dd



## Screenshot display scales nicely


https://github.com/getsentry/sentry/assets/56095982/0ce3e5b4-b455-4fbf-b48b-280e0a151363

## Delete button with confirmation modal


https://github.com/getsentry/sentry/assets/56095982/79ac0349-18cf-49a3-af77-3b779f00d817

## TODOS
- Currently, if you delete a screenshot, it will not show up in UI
anymore. The design specifies that we should instead show this message
instead:
<img width="396" alt="SCR-20240329-kuoq"
src="https://github.com/getsentry/sentry/assets/56095982/dddf4e47-6f1b-4ac8-88c5-e7b43b50628d">

- Pagination code largely copied from the issues component; not tested
since we can't currently add multiple screenshots in a FB submission yet
- Screenshot display in details isn't scaling properly -- if a
screenshot comes in that's extra wide or extra long (e.g. more than
`360px` in either dimension, it'll get cropped to fit within `360px` x
`360px`. However the design specifies that the entire image should
instead scale down to fit within this box, rather than getting cropped
- When clicking into full screen/zoom-in mode, the modal should take up
the whole screen rather than staying the same dimensions as the image.
(see designs for reference)
ryan953 added a commit that referenced this issue Apr 3, 2024
…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)
ryan953 added a commit that referenced this issue Apr 8, 2024
shellmayr pushed a commit that referenced this issue Apr 10, 2024
…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)
@ryan953
Copy link
Member

ryan953 commented Apr 15, 2024

The last part of the design missing is treatment for 'deleted attachments'.

I've spun that off into it's own ticket here: #68907
We can close this one now.

@ryan953 ryan953 closed this as completed Apr 15, 2024
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants