@ashfurrow => Related artworks. #277

alloy · 2015-03-12T22:30:43Z

This fixes #113.

So I decided to take the if-else block out of ARArtworkRelatedArtworksView and make that a controller concern (still to do). The controller then simply adds the sections for the right context and the view doesn’t need to know about it.

Right now my biggest question is about the sale update, it’s not really clear to me how this is working. I assumed it was so that when you’re looking at an auction lot, it keeps the bids up-to-date? But besides it setting a property (saleArtwork) I don’t see how it would do that. Any hints?

alloy · 2015-03-12T22:31:19Z

Btw, I’m gonna cleanup the commits, or maybe even squash, once I’m fully done.

orta · 2015-03-13T00:15:16Z

Yeah the SaleArtwork is used to show a countdown and to provide auction metadata, it gets used here:

eigen/Artsy/Classes/Views/ARArtworkRelatedArtworksView.m

Line 60 in 2e814fc

returnable = self.saleArtwork.auction;

when both networking promises are fulfilled.

ashfurrow · 2015-03-13T16:17:32Z

Artsy Tests/ARArtworkRelatedArtworksViewTests.m

+    UICollectionView *artworksCollectionView = [artworksVCView.subviews lastObject];
+    NSAssert(!CGSizeEqualToSize(artworksCollectionView.frame.size, CGSizeZero),
+             @"There are no visible cells in a UICollectionView if it has no visible frame.");
+    return [[artworksCollectionView visibleCells] valueForKeyPath:@"metadataView.secondaryLabel.text"];


Neat! Might make sore sense to use an Objective-Sugar map: here instead of valueForKeyPath:, for explicitness' sake.

Ah yeah, I keep forgetting about that pod being included, sweet 👌

ashfurrow · 2015-03-13T16:27:15Z

Looks good so far. Very nice on the tests. Don't forget to make indenting consistent – Orta gets fussy.

alloy · 2015-03-13T17:23:04Z

Looks good so far. Very nice on the tests. Don't forget to make indenting consistent – Orta gets fussy.

And rightfully so! I’ll have to setup some project specific vim config. Is it ok if I add that to the repo?

ashfurrow · 2015-03-13T17:23:27Z

Fine with me!

orta · 2015-03-16T15:29:59Z

changelog

alloy · 2015-03-16T22:00:27Z

I’m stuck on how to determine that an artwork was shown in the context of an auction, as the auction views are martsy views and so the artwork is shown with the abstract "/artwork/:id" JLRoute.

Would it be possible to have martsy include a GET parameter that’s only really used by Eigen? E.g.:

https://m.artsy.net/artwork/derrick-adams-head-study-figure-plate-4?auction_id=watermill-center-auction

Or do you have another/better/existing solution?

orta · 2015-03-16T22:01:19Z

the presence of a SaleAuction object means it is in an auction

alloy · 2015-03-16T22:02:26Z

@orta I understand, but that does not indicate if the artwork was shown from an auction context.

orta · 2015-03-16T22:03:49Z

Ah I see, in this case there is no auction context ( not like how we do a fair context ) - the runtime saleartwork lookup is all we use to determine whether to show auction related views.

alloy · 2015-03-16T23:58:00Z

Should be good, functionality-wise. Going to cleanup and go over it once more tomorrow and make it a final PR.

ashfurrow · 2015-03-17T01:46:42Z

Cool!

alloy · 2015-03-17T19:01:52Z

Yeah the SaleArtwork is used to show a countdown and to provide auction metadata

@orta I’m assuming that you meant in another view, e.g. ARArtworkView, because I really don’t see anything related to any of that in this view.

orta · 2015-03-17T19:02:19Z

yeah, deeper in the hierarchy

alloy · 2015-03-17T19:42:59Z

Any clues as to what this is about? Is it about returning, for instance, the saleArtwork here instead? And, if so, what could that impact (seeing as it wasn’t done yet)?

orta · 2015-03-17T20:30:56Z

no idea :D

Described at #113.

* Can’t use `-[MTLModel dictionaryValue]` as it’s not the JSON version. * Can’t use `MTLJSONAdapter` because of all the `null` fields in limited test fixtures.

ashfurrow · 2015-03-18T20:11:47Z

Artsy Tests/ARArtworkRelatedArtworksViewTests.m

    });
 });

+//describe(@"concerning layout", ^{


Meant to be included?

Needed to be fixed, done so now.

ashfurrow · 2015-03-18T20:21:01Z

Generally looks great! A few points. Indenting in the tests is inconsistent (and we don't want to upset Orta). And would this benefit from snapshot tests at all?

Sidetone: I don't love KSPromise, but a bigger conversation needs to happen around our network code in general.

alloy · 2015-03-18T22:06:40Z

Sidetone: I don't love KSPromise, but a bigger conversation needs to happen around our network code in general.

Yeah, agreed on the “we need to have this discussion” part, let’s do that when I’m in NYC.

/cc @1aurabrown @orta

alloy · 2015-03-18T22:50:11Z

Did you mean these inconsistencies c3e1a19 ?

alloy · 2015-03-18T23:09:05Z

And would this benefit from snapshot tests at all?

I thought it wouldn’t, because this is more about logic as to what to show then it is about how it is being showed. I assumed there would already be snapshots for what a grid of (related) artworks look like?

Here’s an example of the snapshots for these tests:

ashfurrow · 2015-03-19T02:05:11Z

Yes, those consistencies.

And you're right about the snapshot tests – we're already testing the constituent parts, so it's coooool.

alloy · 2015-03-19T18:30:32Z

@ashfurrow Anything holding this merge back that I overlooked?

ashfurrow · 2015-03-19T18:34:38Z

Nope - mentioned here that it was good to merge. I'll do so now.

@ashfurrow

@ashfurrow => Related artworks.

orta · 2015-03-19T18:34:51Z

🎉

orta · 2015-03-19T18:35:34Z

also that JSON formatting is fine, I get triggered by things like this:

    if ([UIDevice isPad]) {
        [_userOptions addObject:@{
                  AROptionsKey : AROptionsUseWhiteFolio,
                 AROptionsName : @"White Folio"
        }];
    }

or other, weird don't flow along the left edge things

alloy · 2015-03-19T20:31:40Z

Aiiight, thanks!

alloy · 2015-03-19T20:31:45Z

orta · 2015-03-19T22:22:28Z

( this requires updating some snapshots BTW )
https://eigen-ci.s3.amazonaws.com/snapshots/55092032/index.html

ashfurrow · 2015-03-20T01:51:10Z

Ah yeah, good catch Orta.

alloy · 2015-03-20T15:35:58Z

I must have kept focused testing on :( I'll make a PR tonight.

ashfurrow · 2015-03-20T15:37:58Z

I missed it too – I'm too used to merging on reds that I've stopped verifying why CI failed 😞

alloy · 2015-03-23T11:41:43Z

…ist. Related to #277.

ashfurrow reviewed Mar 13, 2015
View reviewed changes

alloy added 12 commits March 17, 2015 23:52

[Tooling] Add improved -[UIView recursiveDescription].

e253008

[Artwork] Add specs for related artworks.

683ebbb

Described at #113.

[Artwork] Expose a bit more of the view details to more easily test.

278995b

[Artwork] First stab at more works from same fair.

8817053

[Artwork] Get test green for more more works from same fair.

2dbc2aa

* Can’t use `-[MTLModel dictionaryValue]` as it’s not the JSON version. * Can’t use `MTLJSONAdapter` because of all the `null` fields in limited test fixtures.

[Artwork] Refactor test.

fc4d659

[Artwork] Fix fixture data to reflect real world.

5e2afbd

[Artwork] Add works from same show/booth at fair.

5b2e411

[Artwork] Add sections for an artwork in a show.

e8af880

[Artwork] Add section with other works in auction.

b9d28c4

[Artwork] Some cleaning.

a51b354

Add a local vim config that matches the project indentation.

a449316

ashfurrow reviewed Mar 18, 2015
View reviewed changes

alloy mentioned this pull request Mar 18, 2015

Things to discuss IRL artsy/mobile#22

Closed

8 tasks

alloy added 3 commits March 18, 2015 23:14

[Artwork] Make adding related works sections private.

f762977

[Artwork] Reinstate disabled layout tests.

3fea380

[Artwork] Ensure indentation starts at 4 spaces.

c3e1a19

ashfurrow added a commit that referenced this pull request Mar 19, 2015

Merge pull request #277 from artsy/alloy-artwork-related-outside-genes

16fcee0

@ashfurrow => Related artworks.

ashfurrow merged commit 16fcee0 into master Mar 19, 2015

alloy mentioned this pull request Mar 21, 2015

@orta => Update test snapshots of related artworks. #294

Merged

alloy mentioned this pull request Mar 23, 2015

@1aurabrown => Fixes crash caused by assumption that there’s always an artist. #298

Merged

alloy added a commit that referenced this pull request Mar 23, 2015

[Artwork] Fixes crash caused by assumption that there’s always an art…

bb6e152

…ist. Related to #277.

alloy deleted the alloy-artwork-related-outside-genes branch April 1, 2015 16:42

@ashfurrow => Related artworks. #277

@ashfurrow => Related artworks. #277

Conversation

alloy commented Mar 12, 2015

alloy commented Mar 12, 2015

orta commented Mar 13, 2015

ashfurrow Mar 13, 2015

Choose a reason for hiding this comment

alloy Mar 13, 2015

Choose a reason for hiding this comment

ashfurrow commented Mar 13, 2015

alloy commented Mar 13, 2015

ashfurrow commented Mar 13, 2015

orta commented Mar 16, 2015

alloy commented Mar 16, 2015

orta commented Mar 16, 2015

alloy commented Mar 16, 2015

orta commented Mar 16, 2015

alloy commented Mar 16, 2015

ashfurrow commented Mar 17, 2015

alloy commented Mar 17, 2015

orta commented Mar 17, 2015

alloy commented Mar 17, 2015

orta commented Mar 17, 2015

ashfurrow Mar 18, 2015

Choose a reason for hiding this comment

alloy Mar 18, 2015

Choose a reason for hiding this comment

ashfurrow commented Mar 18, 2015

alloy commented Mar 18, 2015

alloy commented Mar 18, 2015

alloy commented Mar 18, 2015

ashfurrow commented Mar 19, 2015

alloy commented Mar 19, 2015

ashfurrow commented Mar 19, 2015

orta commented Mar 19, 2015

orta commented Mar 19, 2015

alloy commented Mar 19, 2015

alloy commented Mar 19, 2015

orta commented Mar 19, 2015

ashfurrow commented Mar 20, 2015

alloy commented Mar 20, 2015

ashfurrow commented Mar 20, 2015

alloy commented Mar 23, 2015