-
Notifications
You must be signed in to change notification settings - Fork 79
Conversation
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.
Overall working pretty well. Probably goes without saying, but when this gets opened, please revert the pod and example app changes.
PinpointKit/PinpointKit/Sources/Core/FeedbackTableViewDataSource.swift
Outdated
Show resolved
Hide resolved
PinpointKit/PinpointKit/Sources/ScreenshotDetector/ScreenshotDetector.swift
Show resolved
Hide resolved
RequestScreenshotCell.swift
Outdated
return | ||
} | ||
|
||
super.addSubview(view) |
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 see that we’re doing this in ScreenshotCell
too, but I’m not sure why we’re not just setting the table view’s separator style to none
🤔. Not necessary to fix for this issue, but I think we should investigate / refactor that later.
PinpointKit/PinpointKit/Sources/Core/FeedbackViewController.swift
Outdated
Show resolved
Hide resolved
let pickerController = PHPickerViewController(configuration: .init(photoLibrary: .shared())) | ||
pickerController.delegate = self | ||
|
||
viewController.present(pickerController, animated: true, completion: nil) |
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.
Any way to limit this to just screenshots via the configuration (none that I’ve seen so far, but maybe there’s an API for a predicate (like we use in screenshot detector) that I’ve missed)? At the very least, we should apply the PHPickerFilter
for images
, as you can currently select videos and live photos.
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 limited it using the filter, but through this API, I'm not sure there's a better way.
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.
Yeah, not really.
Example/PinpointKitExample.xcodeproj/xcshareddata/xcschemes/PinpointKitExample.xcscheme
Outdated
Show resolved
Hide resolved
PinpointKit/PinpointKit/Sources/ScreenshotDetector/ScreenshotDetector.swift
Show resolved
Hide resolved
RequestScreenshotCell.swift
Outdated
private lazy var requestScreenshotButton: UIButton = { | ||
let button = UIButton(type: .system) | ||
button.translatesAutoresizingMaskIntoConstraints = false | ||
button.layer.cornerRadius = DesignConstants.buttonHeight / 2.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.
We’re no longer using buttonHeight
. We may want to just use a constant corner radius that looks good, or apply the corner radius in layoutSubviews()
with the actual button’s height.
RequestScreenshotCell.swift
Outdated
} | ||
|
||
private enum DesignConstants { | ||
static let buttonHeight: CGFloat = 54.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.
Can be removed after the corner radius is addressed.
let pickerController = PHPickerViewController(configuration: .init(photoLibrary: .shared())) | ||
pickerController.delegate = self | ||
|
||
viewController.present(pickerController, animated: true, completion: nil) |
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.
Yeah, not really.
No description provided.