Sharing in discovery card #152
Conversation
| internal final class DiscoveryPostcardCell: UITableViewCell, ValueCell { | ||
| internal weak var delegate: DiscoveryPostcardCellDelegate? { | ||
| didSet { | ||
| self.viewModel.inputs.delegateDidSet() |
There was a problem hiding this comment.
this doesn't seem to be used. is it needed?
| @@ -4,7 +4,16 @@ import Library | |||
| import Prelude | |||
| import UIKit | |||
|
|
|||
| internal protocol DiscoveryPostcardCellDelegate: class { | |||
| func discoveryPostcardTappedShared(shareContext: ShareContext) | |||
There was a problem hiding this comment.
could we add a lil comment to this to the effect of "Called when the share button is tapped".
| @IBOutlet fileprivate weak var socialAvatarImageView: UIImageView! | ||
| @IBOutlet fileprivate weak var socialLabel: UILabel! | ||
| @IBOutlet fileprivate weak var socialStackView: UIStackView! | ||
| @IBOutlet fileprivate weak var starButton: UIButton! | ||
| @IBOutlet fileprivate weak var backgroundGradientView: GradientView! |
There was a problem hiding this comment.
could we alphabetize this?
| super.awakeFromNib() | ||
|
|
||
| self.starButton.addTarget(self, action: #selector(starButtonTapped), | ||
| for: .touchUpInside) |
There was a problem hiding this comment.
would this fit on one line?
| // controller.modalPresentationStyle = .popover | ||
| // let popover = controller.popoverPresentationController | ||
| // popover?.sourceView = self.shareButton | ||
| // } |
There was a problem hiding this comment.
could we get rid of this commented code?
There was a problem hiding this comment.
@cdolm92 have you tested on ipad? this needs to be presented as a popover or it will error.
Library/ShareContext.swift
Outdated
| extension ShareContext: Equatable { | ||
| } | ||
|
|
||
| public func == (lhs: ShareContext, rhs: ShareContext) -> Bool { |
There was a problem hiding this comment.
I think the modern swift way of conforming to equatable is to put the func == inside the extension as a static func ==
Library/ShareContext.swift
Outdated
| return lhs == rhs | ||
| case let (.update(lhs), .update(rhs)): | ||
| return lhs == rhs | ||
| default: |
There was a problem hiding this comment.
is it possible to get rid of the default? we wanna shy away from defaults so that this will trigger a compile error when new cases are added to the enum
| @@ -101,6 +101,20 @@ public let navyButtonStyle = | |||
| <> UIButton.lens.layer.borderColor .~ UIColor.ksr_navy_900.cgColor | |||
| <> UIButton.lens.layer.borderWidth .~ 1.0 | |||
|
|
|||
| public let saveButtonStyle = | |||
There was a problem hiding this comment.
this and shareButtonStyle are some great shared styles!
whatdya think about using these in the project nav bar view too so that we have a single place we are stylin?
|
|
||
| func shareButtonTapped() | ||
|
|
||
| func starButtonTapped() |
| @@ -76,6 +82,9 @@ public protocol DiscoveryPostcardViewModelOutputs { | |||
| /// Emits a boolean to determine whether or not the metadata view should be hidden. | |||
| var metadataViewHidden: Signal<Bool, NoError> { get } | |||
|
|
|||
| /// Emits when we notify the delegate that the share button was tapped. | |||
There was a problem hiding this comment.
the wording on this could be more "Emits when we should notify the delegate..."
|
|
||
| _ = self.starButton | ||
| |> saveButtonStyle | ||
| |> UIButton.lens.hidden .~ true |
There was a problem hiding this comment.
assuming this hidden value is just here because the star hasn't been implemented yet, but just leaving a note to make sure it doesn't end up in bindStyles later!
| // controller.modalPresentationStyle = .popover | ||
| // let popover = controller.popoverPresentationController | ||
| // popover?.sourceView = self.shareButton | ||
| // } |
There was a problem hiding this comment.
@cdolm92 have you tested on ipad? this needs to be presented as a popover or it will error.
|
|
||
| self.shareViewModel.outputs.showShareCompose | ||
| .observeForControllerAction() | ||
| .observeValues { [weak self] in self?.showShareCompose($0) } |
There was a problem hiding this comment.
Can you explain why this was removed? I am forgetting the difference b/w the shareSheet and shareCompose
There was a problem hiding this comment.
We only want to use shareCompose in the ThanksViewController.
| fileprivate let delegateDidSetProperty = MutableProperty() | ||
| public func delegateDidSet() { | ||
| self.delegateDidSetProperty.value = () | ||
| } |
There was a problem hiding this comment.
i know brando already pointed out you aren't using this property in another place, but just marking this bit of code to eliminate as well.
| self.vm.inputs.configureWith(shareContext: .discovery(newProject)) | ||
|
|
||
| self.showShareSheet.assertValueCount(1) | ||
| } |
There was a problem hiding this comment.
you could also assert that the new share context tracking string you added is being passed here.
|
|
WHAT
User can now share a project from the
DiscoveryPostcardCell. In order to make that possible we implemented thediscoveryPostcardTappedShared(shareContext:)delegate method. TheDiscoveryPageViewControlleradheres to this method, presenting the share sheet.