-
Notifications
You must be signed in to change notification settings - Fork 106
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
Attach Screenshot in Bug Report Screen #578
Conversation
Generated by 🚫 Danger Swift against 01eaa40 |
Codecov ReportBase: 48.60% // Head: 48.16% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #578 +/- ##
===========================================
- Coverage 48.60% 48.16% -0.45%
===========================================
Files 262 263 +1
Lines 15067 15110 +43
Branches 9966 9982 +16
===========================================
- Hits 7324 7278 -46
- Misses 7464 7554 +90
+ Partials 279 278 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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 to me. One small comment from the screen recording, could the progress bar say "Sending" instead of loading?
Also should probably ask for a design review on this :)
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.
lgtm too 👍
# Conflicts: # UITests/Sources/BugReportUITests.swift
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.
Hey!! Thanks for working on this. I have a few suggestions/tweaks:
- Is it possible be reduce the size of the text box so that the info/buttons below it are more visible?
- Can we add the 'attach screenshot' button above 'send logs'? It fit's better with the overall heiarchy of this screen.
- Can we add an icon to the left of attach screenshot to make it super obvious what the user is supposed to do here, it also matches with the treatment we are giving buttons within settings.
Figma file for ref:
https://www.figma.com/file/0MMNu7cTOzLOlWb7ctTkv3/Element-X-(new)?node-id=123%3A21855&t=cBA1DmfD4awGuMCW-1
Updated the screen recording in the PR description to show the requested changes. |
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.
Changes look good to me. There's a couple more places where we could use the label style: RoomDetailsScreen
in the aboutSection
and the securitySection
.
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.
💯
ElementX/Sources/Other/SwiftUI/Views/SettingsRowLabelStyle.swift
Outdated
Show resolved
Hide resolved
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
Added an Attach Screenshot button that will add a screenshot to the bug report:
RPReplay_Final1676470242.MP4