-
Notifications
You must be signed in to change notification settings - Fork 1
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
Added additional report info and extended UI customisation #1
Added additional report info and extended UI customisation #1
Conversation
* Added App version, device name, system name and version & user's device locale * Added support for use of dividers to better separate information
…nd submit buttons
Hey 👋 Thanks for putting this library together 🙌 There were a few minor tweaks/additions I needed to make in to integrate nicely into my App. Hope you find these to be useful contributions... |
@@ -1,12 +1,24 @@ | |||
import UIKit |
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 can't check that now but I remember I added plateforme support for MacOS in the Package.swift
. I'm wondering then if this won't break because UIKit can't be imported on MacOS.
Can you have a look ? It's not critical, you can remove MacOS support if needed 😄
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.
Yes, it will. I hadn't noticed there was Mac support in the Package.swift
there are however several existing compilation issues, including existing UIKit imports, use of iOS specific SwiftUI view modifiers which mean it would require a bit more work to get running on Mac. For that reason, I've removed support as per your suggestion.
public init( | ||
email: String, | ||
message: String | ||
) { | ||
self.email = email | ||
self.message = message | ||
|
||
deviceName = UIDevice.current.name |
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 don't think that's useful.
- For devices before iOS16 this will return the user defined name like "Lawmaestro's iPhone 15". I don't want that in order to preserve user's privacy.
- For devices after iOS16, this will only return a generic name, then it's useless considering you also added the
systemName
.
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've updated this to what I actually originally intended it to be which is just a device identifier which can be cross referred to find the device model/info. This of course doesn't have an user PI related info in either ;)
let title: String | ||
let textForegroundColor: Color | ||
|
||
public init( | ||
title: String? = nil, | ||
textForegroundColor: Color? = nil | ||
) { | ||
self.title = title ?? "_send_a_feedback".localized | ||
self.textForegroundColor = textForegroundColor ?? .blue |
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 like there are some indent issues
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.
Tabs vs spaces (the oldest one in the book) :D Switched Xcode over to spaces and reformatted these files so all should be consistent.
Thanks for the contribution and the support! Could you check the few comments I left please ? |
* Replaced device name with model identifier. This provides enough information (e.g. iPhone14,5) to be able to easily cross reference somewhere like https://www.theiphonewiki.com and find the exact device model information. This of course proves useful when wanting to know device characteristics which may relate to an issue. * Removed Mac support. There are several existing compilation issues on Mac already. * Corrected spacing to align with project norms (spaces not tabs)
97737a5
to
d2b675a
Compare
d2b675a
to
e5b36c8
Compare
@lawmaestro sorry for the merge time 🙏🏼 |
No worries, thanks for taking a look 🙌 |
Added additional report information for Notion integration
Extended UI customisation
Localisations