Improved Camera Button UI #39
Conversation
- Move flash button next to camera button - Move cancel button to top left - Move finish button next to camera button
| @@ -133,26 +141,29 @@ final class CameraOverlayView: UIView { | |||
| label.translatesAutoresizingMaskIntoConstraints = false | |||
| NSLayoutConstraint.activate([ | |||
| label.centerXAnchor.constraint(equalTo: self.centerXAnchor), | |||
| label.centerYAnchor.constraint(equalTo: flashButton.centerYAnchor) | |||
| label.centerYAnchor.constraint(equalTo: cancelButton.centerYAnchor) | |||
| ]) | |||
|
|
|||
| return label | |||
| }() | |||
|
|
|||
| private lazy var doneButton: UIButton = { | |||
There was a problem hiding this comment.
nit, could rename this button
| @@ -299,10 +299,19 @@ private extension CameraViewModel { | |||
| resetFocus() | |||
|
|
|||
| if let photoCapture = photoCaptureDelegate.photoCapture { | |||
| var remainingPhotoType: CameraOverlayView.RemainingPhotoType = .none | |||
| if photos.count + 2 <= settings.numberOfPhotos.upperBound { | |||
There was a problem hiding this comment.
nice cleanup with this new enum
nit, maybe naming these in some way would be better since it is kinda confusing that 2 is being added because the current photo isn't in the photo count yet, something like:
let photoCountIncludingCurrentPhoto = photos.count + 1
if photoCountIncludingCurrentPhoto + 1 <= settings.numberOfPhotos.upperBound
if photoCountIncludingCurrentPhoto >= settings.numberOfPhotos.lowerBound
or
let nextPhotoWouldBeBelowMaxPhotoLimit = photos.count + 2 <= settings.numberOfPhotos.upperBound
let currentPhotoIsAboveMinimumPhotoRequirement = photos.count + 1 >= settings.numberOfPhotos.lowerBound
or both/something else
| switch remainingPhotoType { | ||
| case .none: | ||
| saveAndContinueButton.setTitle("Finish", for: .normal) | ||
| finishReviewButton.isHidden = true |
There was a problem hiding this comment.
nit, these two I wasn't sure which was which, I wasn't sure if meant finishing or confirming the photo after reviewing, maybe just finishButton or saveAndFinish to match saveAndContinue
it's a little tricky either way since the continue one can also be finish, but don't have a better name off the top of my head
There was a problem hiding this comment.
Yeah I couldn't come up with good names either :/
updated them to be saveAndCaptureMoreButton and saveAndEndCaptureButton. Really the first one should be like saveAndCaptureMoreIfPossibleButton but that's a little excessive I think
| private func styleButton(_ button: UIButton, outline: Bool, color: UIColor) { | ||
| let backgroundColor: UIColor = color | ||
| let textColor: UIColor = .white | ||
|
|
There was a problem hiding this comment.
nit, think rather than assigning backgroundColor to the text and vice versa in the outline case it would be clearer to just set them directly
button.backgroundColor = outline ? .white : color
button.setTitleColor(outline ? color : .white, for: .normal)
button.layer.borderColor = outline ? color : .white
There was a problem hiding this comment.
Not sure why I tried to semi-preserve the logic from dress code. good call
| if photos.count + 2 <= settings.numberOfPhotos.upperBound { | ||
| if photos.count + 1 >= settings.numberOfPhotos.lowerBound { | ||
| let photoCountIncludingCurrentPhoto = photos.count + 1 | ||
| if photoCountIncludingCurrentPhoto < settings.numberOfPhotos.upperBound { |
There was a problem hiding this comment.
nice catch on the <=, has that always been wrong then?
There was a problem hiding this comment.
Well it used to be +2 <= but I figured it would make more sense to just use the same +1 variable everywhere and change it to <
There was a problem hiding this comment.
wait that's the same logic right or am I trippin
There was a problem hiding this comment.
the one you have now seems the correct one to me, but I think the logic changed because if you had:
upperBound: 1
photosCount: 0
then you would get:
photos.count + 1 < upperBound is true
photos.count + 2 <= upperBound is false
so I guess maybe it made sense the way it was handled before when it was canContinue because you would want false but now that you have .none you would want true
There was a problem hiding this comment.
nvm, 1 is not less than 1, so they are both false, carry on lol 🤦♂️
There was a problem hiding this comment.
😁 It's okay my brain was having trouble too
- Moves the label for the photo to the top of the screen when reviewing a photo to give us more room
- Preview after the photo was cropping landscape photos because it tried to display them in a portrait-oriented view - Fix is to create the preview image with the portrait orientation (even if it is landscape) so that the image doesn't get rotated within our portrait-frame
Pivotal Tracker tickets
https://www.pivotaltracker.com/story/show/177689520
Screenshots
Landscape
Portrait