Skip to content

feat(issue-details): Add screenshot section #27274

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 12, 2021
Merged

feat(issue-details): Add screenshot section #27274

merged 2 commits into from
Jul 12, 2021

Conversation

@priscilawebdev
Copy link
Member

@priscilawebdev priscilawebdev commented Jul 9, 2021 •
edited
Loading

INGEST-136

Previews:

Screenshot 2021-07-09 at 15 35 41
Screenshot 2021-07-09 at 15 22 48

  • Add Screenshot Section
  • Add Empty State
  • Add Download Button
  • Add Documentation Link

will do in another PR:

  • Update code to not fire attachments request twice
  • Add Visualization Modal
  • Add dismiss action (Empty State)
  • Unit and Visual Tests

@priscilawebdev priscilawebdev force-pushed the feat/mobile-screenshots branch from 6f47daa to 225fdd1 Compare July 12, 2021 07:31
@priscilawebdev priscilawebdev force-pushed the feat/mobile-screenshots branch from 225fdd1 to a171ed8 Compare July 12, 2021 07:34
@priscilawebdev priscilawebdev force-pushed the feat/mobile-screenshots branch from a171ed8 to bfb21a5 Compare July 12, 2021 08:21
@priscilawebdev priscilawebdev marked this pull request as ready for review July 12, 2021 08:21
matejminar
matejminar reviewed Jul 12, 2021
View reviewed changes
static/app/components/events/eventEntries.tsx Outdated
Comment on lines 406 to 412
Copy link
Member

@matejminar matejminar Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we'll still want this section to be visible.

static/app/components/events/eventTagsAndScreenshot/screenshot.tsx Outdated
Comment on lines 45 to 47
Copy link
Member

@matejminar matejminar Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do something so that we don't fire this request twice? (both here and in EventAttachments section)

Copy link
Member Author

@priscilawebdev priscilawebdev Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was planning to fix it in a follow up PR

static/app/components/events/eventTagsAndScreenshot/screenshot.tsx Outdated
Comment on lines 57 to 72
Copy link
Member

@matejminar matejminar Jul 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This screenshot preview should work only for images. All other attachments should still be visible/previewable down in the EventAttachments section.

Copy link
Member Author

@priscilawebdev priscilawebdev Jul 12, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Robin design has a video player, so I thought we wanted to support everything. I'll update the code

@priscilawebdev priscilawebdev force-pushed the feat/mobile-screenshots branch from f8e14a2 to 8cb5450 Compare July 12, 2021 09:22
@priscilawebdev priscilawebdev force-pushed the feat/mobile-screenshots branch from 8cb5450 to 4c94a55 Compare July 12, 2021 10:52
@priscilawebdev priscilawebdev requested a review from a team as a code owner July 12, 2021 10:52
@priscilawebdev priscilawebdev force-pushed the feat/mobile-screenshots branch from 4c94a55 to a1789ff Compare July 12, 2021 11:00
@priscilawebdev priscilawebdev force-pushed the feat/mobile-screenshots branch from a1789ff to 971c370 Compare July 12, 2021 11:03
@priscilawebdev priscilawebdev force-pushed the feat/mobile-screenshots branch from 971c370 to ef98513 Compare July 12, 2021 13:07
@priscilawebdev priscilawebdev force-pushed the feat/mobile-screenshots branch from ef98513 to 5342f07 Compare July 12, 2021 13:07
matejminar
matejminar approved these changes Jul 12, 2021
View reviewed changes
Copy link
Member

@matejminar matejminar left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the TODO section in the PR description. It shows nicely what code reviewer shoud/should not focus on :)

Not a blocker:
Image should probably have border radius too.
image

@priscilawebdev priscilawebdev merged commit 511e6dd into master Jul 12, 2021
@priscilawebdev priscilawebdev deleted the feat/mobile-screenshots branch July 12, 2021 13:36
@github-actions github-actions bot locked and limited conversation to collaborators Jul 28, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@matejminar matejminar matejminar approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@priscilawebdev @matejminar