[NT-644] See all project category fix #1061
Conversation
…NT-644-thanks-button-fix
| @@ -3,14 +3,48 @@ import Library | |||
| import Prelude | |||
| import UIKit | |||
|
|
|||
| internal protocol ThanksCategoryCellDelegate: AnyObject { | |||
| func thanksSeeAllProjectsTapped(_ cell: ThanksCategoryCell, category: KsApi.Category) | |||
There was a problem hiding this comment.
How about:
func thanksCategoryCell(_ cell: ThanksCategoryCell, didTapSeeAllProjectsWith category: KsApi.Category)| internal weak var delegate: ThanksCategoryCellDelegate? | ||
| fileprivate let viewModel: ThanksCategoryCellViewModelType = ThanksCategoryCellViewModel() | ||
|
|
||
| @IBOutlet fileprivate var seeAllProjectCategoryButton: UIButton! |
There was a problem hiding this comment.
Some places in this file we use (see all) project singular and other places projects plural. Would be nice to just have one convention.
Also is the name seeAllProjectsButton not more accurate here? Or perhaps seeAllProjectsInCategoryButton (feels a bit long)?
| } | ||
|
|
||
| @objc fileprivate func seeAllProjectCategoryTapped() { | ||
| self.viewModel.inputs.allProjectCategoryButtonTapped() |
There was a problem hiding this comment.
Here there is just an inconsistency between the function name and the input name, so if you could align those to match with all other places that would be cool.
|
|
||
| extension ThanksViewController: ThanksCategoryCellDelegate { | ||
| func thanksSeeAllProjectsTapped(_: ThanksCategoryCell, category: KsApi.Category) { | ||
| self.viewModel.inputs.categoryCellTapped(category) |
There was a problem hiding this comment.
I know this is an existing input but could also be good to align this naming with the delegate method naming once that's updated 🤷♂
| import ReactiveSwift | ||
|
|
||
| public protocol ThanksCategoryCellViewModelInputs { | ||
| func allProjectCategoryButtonTapped() |
There was a problem hiding this comment.
This can also update once the button naming is update to match.
| } | ||
|
|
||
| public protocol ThanksCategoryCellViewModelOutputs { | ||
| var notifyToGoToDiscovery: Signal<KsApi.Category, Never> { get } |
There was a problem hiding this comment.
Can this be notifyDelegateToGoToDiscovery?
| @@ -3,14 +3,48 @@ import Library | |||
| import Prelude | |||
| import UIKit | |||
|
|
|||
| internal protocol ThanksCategoryCellDelegate: AnyObject { | |||
| func thanksSeeAllProjectsTapped(_ cell: ThanksCategoryCell, category: KsApi.Category) | |||
There was a problem hiding this comment.
I think delegate method names usually follow the convention of starting the delegate name with the name of the class that's sending the message:
func thanksCategoryCell(_ cell: ThanksCategoryCell, didTapSeeAllProjectsWith category: KsApi.Category)
| .observeForControllerAction() | ||
| .observeValues { [weak self] in | ||
| guard let _self = self else { return } | ||
| self?.delegate?.thanksSeeAllProjectsTapped(_self, category: $0) |
There was a problem hiding this comment.
Here you could just call _self.delegate?.thanksSeeAllProjectsTapped(_self: category: $0) since you unwrap self in the previous line.
| } | ||
|
|
||
| public protocol ThanksCategoryCellViewModelOutputs { | ||
| var notifyToGoToDiscovery: Signal<KsApi.Category, Never> { get } |
There was a problem hiding this comment.
I wonder if this could be notifyDelegateGoToDiscovery so we're specific about what object is being notified.
| @@ -35,7 +35,7 @@ | |||
| <edgeInsets key="layoutMargins" top="16" left="16" bottom="16" right="16"/> | |||
| <viewLayoutGuide key="safeArea" id="njF-e1-oar"/> | |||
| <connections> | |||
| <outlet property="seeAllProjectsButton" destination="hbb-GW-PrV" id="Sfl-8U-s0p"/> | |||
| <outlet property="seeAllProjectCategoryButton" destination="hbb-GW-PrV" id="RPG-Zr-HaH"/> | |||
There was a problem hiding this comment.
Should this be projects plural? seeAllProjectsCategoryButton 🤔
…NT-644-thanks-button-fix
| self.viewModel.outputs.notifyDelegateToGoToDiscovery | ||
| .observeForControllerAction() | ||
| .observeValues { [weak self] in | ||
| guard let _self = self else { return } |
There was a problem hiding this comment.
By the way, Swift now lets us do guard let self = self else { ... } for this sort of thing.
📲 What
See all [Category] projectsbutton does not work on Thanks Page, this PR fixes that issue.🤔 Why
Used
ThanksCategoryCellDelegateso thatThanksViewControllerdismisses and user sees all projects in a category whenSee all [Category] projectsbutton is tapped.🛠 How
More in-depth discussion of the change or implementation.
👀 See
Trello, screenshots, external resources?
✅ Acceptance criteria
See all [Category] projects. Thanks Screen dismisses you should be at the category explore screen.