artsy / mobile

Mobile Team TODO
http://www.objc.io/issues/22-scale/artsy/
84 stars 11 forks source link

Homework: What could the ArtistViewController look like? #65

Closed orta closed 7 years ago

orta commented 8 years ago

Taking these issues: https://github.com/artsy/eigen/issues?utf8=✓&q=is%3Aissue+label%3A%22Artist+Page+Update%22+

What would be a really optimal way to write this without having to write the entire mplementation. So what would be inside the view controller that could deal with most/all/some of the Artist View. I'd like to see the entire artistVC ideally

Due date: 27th Nov.

orta commented 8 years ago

The Artsy Website does more, and looks like this: https://github.com/artsy/force-public/tree/master/apps/artist

ashfurrow commented 8 years ago

So what I'm thinking is having the Artist VC basically be a very thin wrapper around a collection of components/presentors/whatever. Each component would be responsible for vending out a view to represent its contents, for performing network requests to retrieve more details, and for notifying a delegate about updated contents. It would encapsulate everything from the artist VC, including rotation and handling user interaction.

So I guess the artist view controller would look something like:

func viewDidLoad() {
  super.viewDidLoad()

  self.components = [
    ArtistHeader(self.artist, delegate: self)
    ArtistDetails(self.artist, delegate: self)
  ]
}

I can't even think of what would be in the delegate protocol, since the resizing could be handled automatically like it currently does with ORStackView. I guess navigation away from the page?

protocol ComponentDelegate<C: Component> {
  func component(component: C, userDidNavigateTo url: NSURL)
}

protocol ArtistHeaderDelegate: ComponentDelegate {
}

The detail component could handle interaction and switching between Works/Shows/About/Related. Additionally, the components could wrap child view controllers, so setting components would add the new components as child VCs. We should do something neat to abstract that.

class ComponentContainerController: UIViewController {
  dynamic var components: [Components]! {
    didSet {
      // Take care of configuring VC hierarchy here maybe?
    }
  }
}
orta commented 8 years ago

I was thinking about moving to sit on top of more generic view components and the view controllers job is to assemble them from a collection of well known and tested views.


@interface ARArtistViewController
/// Guaranteed to always exist
@property (nonatomic, copy) NSString *artistID;

// We have no guarantee that this exists on launch
@property (nonatomic, strong) Artist *artist;

// has a tab view that changes infinitely scrollable content below
@property (nonatomic, strong) ARTabbedContentView *view;
@end

@implementation ARArtistViewController

- (UIView *)loadView
{
  return [[ARTabbedContentView alloc] init];
}

- (void)viewDidLoad
{
  [self updateArtist];

  view.header = [StackView stackWithCenteredViews:@[
    [ARTitleLabel labelWithObject:self.artist keypath:@"name"],
    [ARSubTitleLabel labelWithObject:self.artist keypath:@"dateAndLocation"],
    [ARSerifLabel labelWithObject:self.artist keypath:@"followerCount"],
    @(32),
    [ARFollowButton followButtonWithFollowable:self.artistID type:Artist.class]
  ]];

  /// TODO: filter button?

  ARArtistArtworksNetworkModel *artistArtworks = [[ARArtistArtworksNetworkModel alloc] initWithArtistID:self.artistID];
  [view addSection:NSLocalizedString(@"Works", @"Artist Works")
       contentType:ARCollectionViewVerticalMasonry
       displayType:ARCollectionViewStyleArtworkFullMetadata
            titles:@[@"All Works"]
      networkables:@[artistArtworks]];

  // I'd have this "generate" networkmodelable things, as it
  // can probably get all the data it needs for the 3 show sections in one go

  ARArtistShowsNetworkModel *artistShows = [[ARArtistShowsNetworkModel alloc] initWithArtistID:self.artistID];

  [view addSection:NSLocalizedString(@"Shows", @"Artist Shows")
       contentType:ARCollectionViewSections
       displayType:ARCollectionViewStyleShowFullMetadata
            titles:@[@"Solo Shows", @"Group Shows", @"Fair Booths"]
      networkables:@[[artistShows solo], [artistShows groups], [artistShows fairs]];

   ARArtistArticlesNetworkModel *artistArticles = [[ARArtistArticlesNetworkModel alloc] initWithArtistID:self.artistID];

   [view addSection:NSLocalizedString(@"About", @"About the Artist title")
        contentType:ARCollectionViewStack
              stack:@[
        [ARSection title:NSLocalizedString(@"Biography", @"Artists inline Biography") views:@[
          [ARTextView textViewWithObject:self.artist markdownKeypath:@"bio"]
        ]];

        // A Network Section wouldn't show title if there wasn't any articles to show for example
        // could be the base of the `ARCollectionViewSections`

        [ARNetworkSection title:NSLocalizedString(@"Articles", @"Artists inline Articles")
                    contentType:ARCollectionViewArticles
                    displayType:ARCollectionViewStyleArticlePreview
                   networkables:@[artistArticles]];
    ]];

   ARArtistRelatedNetworkModel *artistRelated = [[ARArtistRelatedNetworkModel alloc] initWithArtistID:self.artistID];

   [view addSection:NSLocalizedString(@"Related", @"Artist Shows")
        contentType:ARCollectionViewSections
        displayType:ARCollectionViewStyleModelablePreviewImages
             titles:@[@"Related Artists", @"Related Categories"]
       networkables:@[[artistRelated artists], [artistRelated categories]];
}

...
@end

Assuming all of the components hook up correctly, this would create a pretty stateless and simple view controller. Meaning that we can ship view controllers faster, and without spending so much time building view controllers with tonnes of custom logic.

It'd rely on some kind of standard for network models and pagination. In writing this I also started thinking a lot about how the network model could create things that are easily paginated and updatable. They are a good example for an Rx style observable. It'd also need some kind of binding glue around updating when the model object changes updates, I'd rather it wasn't actual raw KVO, but promises/KVOObservers/whatever is fine, no strong opinions here.

I obviously have no strong opinions on about swift/objc too

ashfurrow commented 8 years ago

Yeah, I like this.

About the promises, I agree. We could pretty easily create extensions for signals/observables/KVOwhatever to a protocol that we could consume. That way, the individual host app can decided which way works best for it.

alloy commented 8 years ago

@orta I really like this :+1:

And it nicely opens up the possibility for the one thing that I was more interested in in my example, which is coalescing the network calls and get us some of that sweet GraphQL action going.

So instead of just having network models that fetch a complete payload for a single model, I’d want these network models to have the actual properties that it exposes. The view that uses a network model calls some promises on the models for the data it wants and the model records those calls. The root view should then be able to get the complete subtree of network models and the recorded calls and create a single GraphQL request out of it.

@interface ARNetworkModel : NSObject

@property (assign) BOOL needsRequest;
@property (weak) UIView *view;

- (void)requestIfNeeded;

@end

@interface ARNetworkModel ()
@property (copy) NSDictionary<NSString *, Future *> *requiredProperties;
@end

@implementation ARNetworkModel

- (instancetype)init;
{
  if ((self = [super init])) {
    _requiredProperties = @{};
    _needsRequest = YES;
  }
  return self;
}

- (void)requestIfNeeded;
{
  NSDictionary *propertiesWithDependentModels = [self propertyRequirementsOfSubtree];
  NSSet *properties = [NSSet setWithArray:propertiesWithDependentModels.allKeys];
  NSSet *models = [NSSet setWithArray:propertiesWithDependentModels.allValues];

  // Generates a GraphQL request for keypaths like `artist.name` or `artist.artworks.name` etc.
  [ArtsyAPI performGraphQLRequestForPropertyKeyPaths:properties completed:^(NSDictionary *payload) {
    // Let each model take the data they require and fulfil their emitted futures.
    for (ARNetworkModel *model in models) {
      [model assignPropertiesFromPayload:payload];
    }
  }];
}

// Get list of required properties for all the subviews’ network models that are marked with `needsRequest`.
// { @"artist.name": @[subtreeModel1, subtreeModel2] }
- (NSDictionary<NSString *, NSArray<ARNetworkModel *> *> *)propertyRequirementsOfSubtree;
{
  // Do some recursive stuff...
  return requirements;
}

- (void)assignPropertiesFromPayload:(NSDictionary *)payload;
{
  for (NSString *property in self.requiredProperties) {
    [self.requireProperties[property] fulfilWithValue:payload[property]];
  }
  self.needsRequest = NO;
}

@end

@interface ARArtistNetworkModel : ARNetworkModel
@property (readonly) Future *name;
@end

@implementation ARArtistNetworkModel

// A network model only has properties that return futures. It should expose those that are required by the component
// that this network model belongs to.
//
// Network models lower in the tree might expose the same property, that’s fine, they’ll all get the same data.
//
// @note These implementations should be generated dynamically based on a list of properties that a model exposes.
//
- (Future *)name;
{
  Future *future = self.requiredProperties[@"artist.name"];
  if (future == nil) {
    future = [Future new];
    NSMutableDictionary *properties = [self.requiredProperties mutableCopy];
    properties[@"artist.name"] = future;
    self.requiredProperties = properties;
  }
  return future;
}

@end

@interface UIView (ARNetworkable)

// Each view can have its own network model that records the properties that that component depends on.
//
- (ARNetworkModel *)ar_networkModel;

@end

@implementation ARArtistViewController

- (void)viewDidLoad;
{
  // This is all identical to Orta’s example, except that the various ‘sections’ should be views that are
  // associated with their views through `-[ARNetworkModel setView:]`, so that the network models are able to traverse
  // the tree and find submodels.

  ARNetworkModel *artistNetworkModel = [[ARArtistNetworkModel alloc] init];
  self.view.ar_networkModel = artistNetworkModel;
  [self.view.ar_networkModel requestIfNeeded];
}

This prototype does side-step models like Artist etc completely, though, so shared behaviour would be lost. Maybe all the properties on those models should be the futures and the network models get the recorded required properties from there?

alloy commented 8 years ago

With regards to JSON parsing in this brave new ‘GraphQL with network model tree’ world, I suggest we use this lean lib by @krzysztofzablocki instead of Mantle: https://github.com/krzysztofzablocki/KZPropertyMapper.

orta commented 8 years ago

I've had a chat with @broskoski - they're using metaphysics for the live auctions API, he's got a workin-ish build ATM. They made graphQL requests on each for request, so they don't see issues there either.

orta commented 8 years ago

/cc @chriseidhof who I mentioned this to, too

chriseidhof commented 8 years ago

I didn't read everything in detail, but my idea was that you could have each component return a GraphQL query combined with a parsing functions. These then get combined into one big GraphQL query, and one big parsing function. I could cook up a prototype, but have to set up a GraphQL server first... Maybe it's easier to do this at the office this week, I can sit down with someone on the team and try to explain my idea and we can build it together. Obviously I don't have any overview of your actual constraints and set-up...

alloy commented 8 years ago

@chriseidhof Hey, thanks for the offer! I’d love to look into the prototyping together with you, but I’ll need a few days for other things first, would you have time on Thursday or (even better) Friday? I’ll also look into setting up local test GraphQL server before then.

And while I can imagine it being really really hard, could I ask you to completely hold-off prototyping this until then? Because I’d like to be involved from the start of the implementation.

chriseidhof commented 8 years ago

I'm not sure what next will look like, but sure, ping me! I don't think my solution would feel native to ObjC, though.

alloy commented 8 years ago

Oh no, we're doing this full on hipster style in Swift.

ashfurrow commented 8 years ago

orta commented 7 years ago

We found the answer to this, react-native + relay.