Add Basic Food Table View #66

lokae0 · 2019-01-28T02:09:18Z

Add Food Table View class cluster and unit tests.

For now, DetailViewModel passes in the array of Food items. In an upcoming PR, this will be moved to a mutable datastore.

This PR also updates protocols that constrain to class to use AnyObject instead, per Swift 4 convention. class will likely be deprecated in a future Swift version. See https://github.com/apple/swift-evolution/blob/master/proposals/0156-subclass-existentials.md#class-and-anyobject

…nstraints

lokae0 · 2019-01-28T02:17:51Z

Application/Tests/Stubs/StubDetailPresenter.swift

@@ -11,6 +11,10 @@ class StubDetailPresenter: StubSelectionPresenter {

 extension StubDetailPresenter: DetailPresenter {

+    func foodTablePresentation(of viewModel: FoodTableViewModel) -> DismissablePresentation {
+        return DismissablePresentation.stub()
+    }


Had to include this because StubDetailPresenter can't also inherit from StubFoodTablePresenter in addition to StubSelectionPresenter. They are both classes and not protocols. Any thoughts or ideas on this?

(Maybe fine because it's just for testing?)

Ah, yeah... This should probably either be done with protocol extensions instead (or just implemented uniquely in every stub) rather than using class inheritance. What you have here is fine for this PR if you'd prefer to punt on that.

n8chur · 2019-02-01T18:48:50Z

Application/Source/Detail/Navigation/DetailNavigationController.swift

+
+}
+
+protocol DetailNavigationControllerFactoryProtocol: DetailViewControllerFactoryProtocol, FoodTableViewControllerFactoryProtocol, SingleViewNavigationControllerFactoryProtocol { }


Should DetailViewControllerFactoryProtocol conform to FoodTableViewControllerFactoryProtocol (and therefore remove the need to specify the conformance here)?

n8chur · 2019-02-01T18:51:33Z

Application/Source/Food Table/FoodTableViewController.swift

+    @available(*, unavailable)
+    required init?(coder aDecoder: NSCoder) { fatalError("\(#function) not implemented.") }
+
+    private let cellIdentifier = "FoodCell"


Nit: Should this just be defined in viewDidLoad since it's not referenced anywhere else?

Alternatively, should this be static?

n8chur · 2019-02-01T18:54:57Z

Application/Source/Food Table/FoodTableViewControllerStyle.swift

+import UIKit
+import Core
+
+struct FoodTableViewControllerStyle: Style {


I haven't thought about how theming with cells using the rx style UITableView would work.

I believe we'd have to iterate over all visible cells when it's set and update the style using a ...CellStyle, and also have the current style available to the tableView.rx.items(cellIdentifier: cellIdentifier) binding in the UIViewController's viewDidLoad function.

Is this something you'd like to try implementing yourself? We could pair program it?

n8chur · 2019-02-01T18:57:20Z

Application/Source/Food Table/FoodTableViewModel.swift

+class FoodTableViewModel: ViewModel {
+
+    let isActive = BehaviorRelay(value: false)
+    let foods: BehaviorRelay<[Food]>


Should this be exposed as Property<[Food]> so that it's not mutable by consumers of this ViewModel?

n8chur · 2019-02-01T18:58:37Z

Application/Source/Food Table/FoodTableViewPresenter.swift

+        withFactory factory: FoodTableViewModelFactoryProtocol,
+        foods: BehaviorRelay<[Food]>,
+        setupViewModel: ((FoodTableViewModel) -> Void)? = nil
+        ) -> Action<Bool, FoodTableViewModel> {


Nit: This should be un-indented one line.

n8chur · 2019-02-01T19:02:01Z

Application/Source/Food Table/FoodTableViewPresenter.swift

+    ///             is executed. Consumers can use this to observe changes to the presented view model if necessary.
+    func makePresentFoodTable(
+        withFactory factory: FoodTableViewModelFactoryProtocol,
+        foods: BehaviorRelay<[Food]>,


Foods should be owned by the FoodTableViewModelFactoryProtocol and should not need to be passed in here. The factory is responsible for owning and injecting all dependencies.

n8chur · 2019-02-01T19:05:30Z

Application/Source/Food Table/FoodTableViewModel.swift

+import Presentations
+import Action
+
+class FoodTableViewModel: ViewModel {


Nit: I'd prefer not to have Table in the name of the ViewModel (or ViewController unless there is also a Collection version as well).

Should this instead be called this IngredientsViewModel? FoodListViewModel? FoodViewModel?

n8chur · 2019-02-01T19:07:29Z

Application/Source/Food Table/FoodTableViewModel.swift

+}
+
+protocol FoodTableViewModelFactoryProtocol {
+    func makeFoodTableViewModel(with foods: BehaviorRelay<[Food]>) -> FoodTableViewModel


foods should be a property of FoodTableViewModelFactoryProtocol—not passed in—since it is a dependency of FoodTableViewModel.

We should also make foods a property of the DetailViewModelFactoryProtocol, and then both would inject foods into their corresponding view models when they are created.

n8chur · 2019-02-01T19:10:31Z

Application/Tests/Stubs/StubDetailPresenter.swift

@@ -11,6 +11,10 @@ class StubDetailPresenter: StubSelectionPresenter {

 extension StubDetailPresenter: DetailPresenter {

+    func foodTablePresentation(of viewModel: FoodTableViewModel) -> DismissablePresentation {
+        return DismissablePresentation.stub()
+    }


Ah, yeah... This should probably either be done with protocol extensions instead (or just implemented uniquely in every stub) rather than using class inheritance. What you have here is fine for this PR if you'd prefer to punt on that.

n8chur · 2019-02-01T19:13:51Z

Application/Source/Detail/View/DetailViewModel.swift

@@ -50,24 +48,26 @@ class DetailViewModel: ViewModel, SelectionPresentingViewModel {
                .disposed(by: self.disposeBag)
        })

-    init(selectionFactory: SelectionViewModelFactoryProtocol) {
+    init(selectionFactory: SelectionViewModelFactoryProtocol, foodTableFactory: FoodTableViewModelFactoryProtocol) {


Do you think this should be a single factory: SelectionViewModelFactoryProtocol & FoodTableViewModelFactoryProtocol (perhaps typealias'd as Factory in the scope of this ViewModel) parameter instead? I can't imagine a case where you would be passing two separate instances here...

lokae0 · 2019-02-16T02:31:49Z

@n8chur PR updated. Let's chat IRL if there are any other suggested changes, as I won't spend much more time on this going forward. Travel prep and my other side project beckons!

n8chur · 2019-02-23T19:02:54Z

Application/Source/Detail/View/DetailViewController.swift

@@ -81,4 +81,8 @@ extension DetailViewControllerFactoryProtocol {
        return DetailViewController(viewModel: viewModel, themeProvider: themeProvider)
    }

+    func makeFoodInfoViewController(viewModel: FoodInfoViewModel) -> FoodInfoViewController {


I don't believe this is needed. This function should be implemented by the protocol extension.

n8chur · 2019-02-23T19:10:15Z

Application/Source/Detail/View/DetailViewModel.swift

    func makeDetailViewModel() -> DetailViewModel
 }

 extension DetailViewModelFactoryProtocol {

+    // This can be backed by a datastore. For now, use a computed property to represent persistent data
+    var foods: Property<[Food]> {


Perhaps this should just be defined on the class that implements DetailViewModelFactoryProtocol (DetailNavigationModelFactory). Even though this is temporary, it's odd that it would instantiate the data model in a protocol extension. It could look like this (in DetailNavigationModel.swift):

class DetailNavigationModelFactory: DetailNavigationModelFactoryProtocol { let foods = Property([.beans, .greens, .potatoes, .tomatoes]) }

I believe one of the Stubs would also need to be updated.

* Add content title and button to Details view (#60) * Added strings for 'Contents' on details view * Change simulated delay to 1.0 to be more noticable * Clarify presenting VM vs presented VM in comments * Added content list title and button to detail view * Add some breathing room to stackViews * Fix unit test timeout * Add Content model and list display (#63) * Abstract centered label, remove unnecessary button boilerplate, and add spacing view * No longer excluding colors.txt from project in Xcode * Add alternate body text color * Adding content / food enum for testing * Display contents in detail view * Further abstract contents list creation and add unit test * Refactor contents list to be more reactive * Changes per PR review * Remove centeredLabel extension in favor of UILabelStyle additions * Use container stackView instead of spacingView * Add Basic Food Table View (#66) * Add FoodTable class cluster * Populate basic tableView * Add tests for FoodTable * Per Swift4 convention, use AnyObject instead of class for protocol constraints * Per PR review, fix FoodVCFactoryProtocol conformance * Per PR review, rename FoodTable to FoodInfo * Per PR review, streamline DetailVM factory protocol parameter * Per PR review, make cell identifier local var * Per PR review, update foods dependency structure * PR cleanup * Per PR review, cleanup where Food is located

lokae0 added 4 commits January 27, 2019 18:04

Add FoodTable class cluster

c08377e

Populate basic tableView

b8b54f6

Add tests for FoodTable

8d32dfa

Per Swift4 convention, use AnyObject instead of class for protocol co…

3c38a7c

…nstraints

lokae0 requested a review from n8chur January 28, 2019 02:15

lokae0 commented Jan 28, 2019

View reviewed changes

n8chur requested changes Feb 1, 2019

View reviewed changes

lokae0 added 6 commits February 15, 2019 16:50

Per PR review, fix FoodVCFactoryProtocol conformance

5555e88

Per PR review, rename FoodTable to FoodInfo

24e1ba3

Per PR review, streamline DetailVM factory protocol parameter

bce7ee5

Per PR review, make cell identifier local var

bf6279f

Per PR review, update foods dependency structure

3bba1d2

PR cleanup

c0da432

n8chur approved these changes Feb 23, 2019

View reviewed changes

Per PR review, cleanup where Food is located

fd55730

lokae0 force-pushed the food_table branch from 7854df4 to fd55730 Compare February 25, 2019 01:34

lokae0 merged commit a0e3b73 into contents_feature Feb 25, 2019

lokae0 deleted the food_table branch February 25, 2019 01:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Basic Food Table View #66

Add Basic Food Table View #66

lokae0 commented Jan 28, 2019 •

edited

Loading

lokae0 Jan 28, 2019

lokae0 Jan 28, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

n8chur Feb 1, 2019

lokae0 commented Feb 16, 2019

n8chur Feb 23, 2019

n8chur Feb 23, 2019


		}

		protocol DetailNavigationControllerFactoryProtocol: DetailViewControllerFactoryProtocol, FoodTableViewControllerFactoryProtocol, SingleViewNavigationControllerFactoryProtocol { }

Add Basic Food Table View #66

Add Basic Food Table View #66

Conversation

lokae0 commented Jan 28, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokae0 commented Feb 16, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lokae0 commented Jan 28, 2019 •

edited

Loading