mhacks / mhacks-ios

The official MHacks iOS app
https://mhacks.org
22 stars 6 forks source link

Create dedicated coordinator to manage view controller transitions #29

Open konnork opened 7 years ago

konnork commented 7 years ago

Depends on #26

Currently we are using storyboards with segues to manage view controller transitions

I propose moving to a dedicated coordinator object to handle transitions between view controllers. This AppCoordinator will be instantiated in the AppDelegate and manage all view controllers. It's initializer will create the initial view controller hierarchy (e.g. UITabBarController -> UINavigationController -> UIViewController (x5)).

It will have a hierarchical coordination system:

AppCoordinator: CalendarCoordinator MapsCoordinator CountdownCoordinator AnnouncementsCoordinator TicketCoordinator

Each view controller with transition activating events (like tapping a table view cell) will have its own coordination delegate defined with methods such as calendarViewController:didSelectEvent: that will be implemented by its designated coordinator to manage the instantiation and transition to EventViewController with that particular event.

Goals

konnork commented 7 years ago

@russellladd @manavgabhawala Thoughts?

russellladd commented 7 years ago

I don't understand why this is necessary.

First point: View controllers should already be oblivious to their surrounding context (i.e. they don't know how they got on screen). View controllers are responsible for getting other view controllers on screen (downstream view controllers), but I don't see a good reason to separate this concern into it's own class.

Second and third points: I think getting rid of storyboards should already go a long way to making these achievable, no?

I do think there is an interesting idea here, especially since I assume the CalendarCoordinator would wrap the UINavigationController + CalendarViewController + EventViewController all into one package.

I feel like this would only serve to add 6 more classes and shuffle code around. Moreover, I don't think it would make the code easier to understand for us, and it would definitely make it harder to onboard new people.

manavgabhawala commented 7 years ago

I don't see the purpose of making a distinction between VCs and coordinators. How are they different?

Also just from the goals:

konnork commented 7 years ago

My bad I wasn't clear, when I said surrounding context, I exclusively was referring to downstream view controllers. Personally, I like to prevent view controllers from doing anything besides setting up their view and responding to events (and delegating anything they don't have complete ownership of). I admit I'm a bit on the extreme side on that.

Your point on tests is completely valid, I'm going to make a (inevitably unsuccessful) push for testing this summer, so we will see what happens there

Saying they don't "serve any purpose" might be a bit harsh. I admit it isn't currently hindering development at all, I just find it cleaner.